[arm/aspeed/ast2500,v1] ipmi: add an Aspeed KCS IPMI BMC driver

The KCS (Keyboard Controller Style) interface is used to perform in-band
IPMI communication between a server host and its BMC (BaseBoard Management
Controllers).

This driver exposes the KCS interface on ASpeed SOCs (AST2400 and AST2500)
as a character device. Such SOCs are commonly used as BMCs and this driver
implements the BMC side of the KCS interface.

Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com>
---
 .../devicetree/bindings/ipmi/aspeed-kcs-bmc.txt    |  26 +
 drivers/char/ipmi/Kconfig                          |   9 +
 drivers/char/ipmi/Makefile                         |   1 +
 drivers/char/ipmi/kcs-bmc.c                        | 744 +++++++++++++++++++++
 include/uapi/linux/kcs-bmc.h                       |  14 +
 5 files changed, 794 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
 create mode 100644 drivers/char/ipmi/kcs-bmc.c
 create mode 100644 include/uapi/linux/kcs-bmc.h

On 01/16/2018 05:43 AM, Haiyue Wang wrote:
> The KCS (Keyboard Controller Style) interface is used to perform in-band
> IPMI communication between a server host and its BMC (BaseBoard Management
> Controllers).
>
> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and AST2500)
> as a character device. Such SOCs are commonly used as BMCs and this driver
> implements the BMC side of the KCS interface.

I thought we were going to unify the BMC ioctl interface?  My preference 
would be to
create a file named include/uapi/linux/ipmi-bmc.h and add the following:

#define __IPMI_BMC_IOCTL_MAGIC    0xb1
#define IPMI_BMC_IOCTL_SMS_SET_ATN    _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)

to make it the same as BT.  Then in bt-bmc.h, set BT_BMC_IOCTL_SMS_ATN to
IPMI_BMC_IOCTL_SMS_SET_ATN.  Then add the KCS ioctls in ipmi-bmc.h and
use that.  That way we stay backward compatible but we are unified.

Since more KCS interfaces may come around, can you make the name more
specific?  (I made this same error on bt-bmc,c, it should probably be 
renamed.)

More comments inline, as I'll go ahead and review this.

> Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com>
> ---
>   .../devicetree/bindings/ipmi/aspeed-kcs-bmc.txt    |  26 +
>   drivers/char/ipmi/Kconfig                          |   9 +
>   drivers/char/ipmi/Makefile                         |   1 +
>   drivers/char/ipmi/kcs-bmc.c                        | 744 +++++++++++++++++++++
>   include/uapi/linux/kcs-bmc.h                       |  14 +
>   5 files changed, 794 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
>   create mode 100644 drivers/char/ipmi/kcs-bmc.c
>   create mode 100644 include/uapi/linux/kcs-bmc.h
>
> diff --git a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
> new file mode 100644
> index 0000000..dd0c73d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
> @@ -0,0 +1,26 @@
> +* Aspeed KCS (Keyboard Controller Style) IPMI interface
> +
> +The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs
> +(BaseBoard Management Controllers) and the KCS interface can be
> +used to perform in-band IPMI communication with their host.
> +
> +Required properties:
> +- compatible : should be one of
> +    "aspeed,ast2400-kcs-bmc"
> +    "aspeed,ast2500-kcs-bmc"
> +- interrupts : interrupt generated by the controller
> +- kcs_chan : The LPC channel number in the controller
> +- kcs_addr : The host CPU IO map address
> +
> +
> +Example:
> +
> +    kcs3: kcs3@0 {
> +        compatible = "aspeed,ast2500-kcs-bmc";
> +        reg = <0x0 0x80>;
> +        interrupts = <8>;
> +        kcs_chan = <3>;
> +        kcs_addr = <0xCA2>;
> +        status = "okay";
> +    };
> +
> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> index 3544abc..36132f8 100644
> --- a/drivers/char/ipmi/Kconfig
> +++ b/drivers/char/ipmi/Kconfig
> @@ -104,3 +104,12 @@ 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
> +	select REGMAP_MMIO
> +	tristate "KCS IPMI bmc driver"
> +	help
> +	  Provides a driver for the KCS (Keyboard Controller Style) IPMI
> +	  interface found on Aspeed SOCs (AST2400 and 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 33b899f..f217bae 100644
> --- a/drivers/char/ipmi/Makefile
> +++ b/drivers/char/ipmi/Makefile
> @@ -22,3 +22,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..d6eab0b
> --- /dev/null
> +++ b/drivers/char/ipmi/kcs-bmc.c
> @@ -0,0 +1,744 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2015-2018, Intel Corporation.
> +
> +#include <linux/atomic.h>
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kcs-bmc.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/slab.h>
> +#include <linux/timer.h>
> +
> +#define KCS_MSG_BUFSIZ      1024
> +#define KCS_CHANNEL_MAX     4
> +
> +/*
> + * 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

It would be nicer to make the above an enum.

> +
> +/* Abort Phase */
> +#define ABORT_PHASE_ERROR1      0x01
> +#define ABORT_PHASE_ERROR2      0x02

Can the above just be folded into two separate phases in kcs_phase?
That would be a little cleaner.


> +
> +/* 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             BIT(3)
> +#define KCS_STR_SMS_ATN             BIT(2)
> +#define KCS_STR_IBF                 BIT(1)
> +#define KCS_STR_OBF                 BIT(0)
> +
> +
> +/* mapped to lpc-bmc@0 IO space */
> +#define LPC_HICR0            0x000
> +#define     LPC_HICR0_LPC3E          BIT(7)
> +#define     LPC_HICR0_LPC2E          BIT(6)
> +#define     LPC_HICR0_LPC1E          BIT(5)
> +#define LPC_HICR2            0x008
> +#define     LPC_HICR2_IBFIF3         BIT(3)
> +#define     LPC_HICR2_IBFIF2         BIT(2)
> +#define     LPC_HICR2_IBFIF1         BIT(1)
> +#define LPC_HICR4            0x010
> +#define     LPC_HICR4_LADR12AS       BIT(7)
> +#define     LPC_HICR4_KCSENBL        BIT(2)
> +#define LPC_LADR3H           0x014
> +#define LPC_LADR3L           0x018
> +#define LPC_LADR12H          0x01C
> +#define LPC_LADR12L          0x020
> +#define LPC_IDR1             0x024
> +#define LPC_IDR2             0x028
> +#define LPC_IDR3             0x02C
> +#define LPC_ODR1             0x030
> +#define LPC_ODR2             0x034
> +#define LPC_ODR3             0x038
> +#define LPC_STR1             0x03C
> +#define LPC_STR2             0x040
> +#define LPC_STR3             0x044
> +
> +/* mapped to lpc-host@80 IO space */
> +#define LPC_HICRB            0x080
> +#define     LPC_HICRB_IBFIF4         BIT(1)
> +#define     LPC_HICRB_LPC4E          BIT(0)
> +#define LPC_LADR4            0x090
> +#define LPC_IDR4             0x094
> +#define LPC_ODR4             0x098
> +#define LPC_STR4             0x09C
> +
> +
> +/* IPMI 2.0 - 9.5, KCS Interface Registers */
> +struct kcs_ioreg {
> +	u32 idr; /* Input Data Register */
> +	u32 odr; /* Output Data Register */
> +	u32 str; /* Status Register */
> +};
> +
> +static const struct kcs_ioreg kcs_ioreg_map[KCS_CHANNEL_MAX] = {
> +	{ .idr = LPC_IDR1, .odr = LPC_ODR1, .str = LPC_STR1 },
> +	{ .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
> +	{ .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
> +	{ .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
> +};
> +
> +struct kcs_bmc {
> +	struct regmap *map;
> +	spinlock_t     lock;

This lock is only used in threads, as far as I can tell.  Couldn't it 
just be a normal mutex?
But more on this later.

> +
> +	u32 chan;
> +	int running;
> +
> +	u32 idr;
> +	u32 odr;
> +	u32 str;
> +
> +	int kcs_phase;
> +	u8  abort_phase;
> +	u8  kcs_error;
> +
> +	wait_queue_head_t queue;
> +	int  data_in_avail;

data_in_avail should be a bool.

You set data_in_avail after the data is ready, but you don't have a 
barrier.  You
have similar problems with kcs_phase.

In fact, the locking in the driver doesn't seem quite correct.  If this 
ever ran on
an SMP system, it is likely to not work correctly.  You can assume that 
the interrupt
runs exclusively, but you cannot assume that the data operations are 
available in
order on another processor that handles a subsequent interrupt.

The easiest way to fix this would be to add the spin lock around the 
case statement
in the irq handler and add them in the poll and read functions (you 
would need to
leave it as a spinlock in that case).  That would provide the proper 
barriers assuming
you put them in the right place (checking data_in_avail again inside the 
spinlock
in the read case, for instance).

> +	int  data_in_idx;
> +	u8  *data_in;
> +
> +	int  data_out_idx;
> +	int  data_out_len;
> +	u8  *data_out;
> +
> +	struct miscdevice miscdev;
> +};
> +
> +static u8 kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> +{
> +	u32 val = 0;
> +	int rc;
> +
> +	rc = regmap_read(kcs_bmc->map, reg, &val);
> +	WARN(rc != 0, "regmap_read() 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, reg, data);
> +	WARN(rc != 0, "regmap_write() failed: %d\n", rc);
> +}
> +
> +static inline void kcs_set_state(struct kcs_bmc *kcs_bmc, u8 state)
> +{
> +	regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_STATE_MASK,
> +			KCS_STR_STATE(state));
> +}
> +
> +static inline void kcs_set_atn(struct kcs_bmc *kcs_bmc)
> +{
> +	regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
> +			KCS_STR_SMS_ATN);
> +}
> +
> +static inline void kcs_clear_atn(struct kcs_bmc *kcs_bmc)
> +{
> +	regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
> +			0);
> +}
> +
> +/*
> + * 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
> + */
> +static void kcs_set_addr(struct kcs_bmc *kcs_bmc, u16 addr)
> +{
> +	switch (kcs_bmc->chan) {
> +	case 1:
> +		regmap_update_bits(kcs_bmc->map, LPC_HICR4,
> +				LPC_HICR4_LADR12AS, 0);
> +		regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
> +		regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
> +		break;
> +
> +	case 2:
> +		regmap_update_bits(kcs_bmc->map, LPC_HICR4,
> +				LPC_HICR4_LADR12AS, LPC_HICR4_LADR12AS);
> +		regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
> +		regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
> +		break;
> +
> +	case 3:
> +		regmap_write(kcs_bmc->map, LPC_LADR3H, addr >> 8);
> +		regmap_write(kcs_bmc->map, LPC_LADR3L, addr & 0xFF);
> +		break;
> +
> +	case 4:
> +		regmap_write(kcs_bmc->map, LPC_LADR4, ((addr + 1) << 16) |
> +			addr);
> +		break;
> +
> +	default:

Shouldn't you return an error here?

> +		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, 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);
> +		}

The above shouldn't have {}, did you run this through checkpatch?

> +		break;
> +
> +	default:

Error here, too?

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

This is identical to the default.  And the default should never happen, 
anyway.
If the default happens you have a software bug and need to report it.

> +		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);

Wouldn't you want to read the command before you write the OBF?

> +	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);

You don't set data_in_avail() to zero here?

> +}
> +
> +static irqreturn_t kcs_bmc_irq(int irq, void *arg)
> +{
> +	struct kcs_bmc *kcs_bmc = arg;
> +	u32 sts;
> +
> +	if (regmap_read(kcs_bmc->map, kcs_bmc->str, &sts) != 0)
> +		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 irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	return devm_request_irq(dev, irq, kcs_bmc_irq, IRQF_SHARED,
> +			dev_name(dev), kcs_bmc);
> +}
> +
> +
> +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)
> +{
> +	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
> +	unsigned long flags;
> +
> +	if (kcs_bmc->running)
> +		return -EBUSY;
> +

The above is a race, it needs to be done inside the lock.

> +	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);

What happens if the interface is not in a state where it can send a message?
The release code doesn't block until anything is done, so the interface 
might
not be in a place where you can use it.  I think the init code handles 
it from
that side, but the release side is not handled.

Also, if release gets called, wouldn't you want to call 
kcs_force_abort() here
or in release()? That would be the equivalent of the BMC getting reset.

> +
> +	return 0;
> +}
> +
> +static unsigned int kcs_bmc_poll(struct file *filp, poll_table *wait)
> +{
> +	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
> +	unsigned int mask = 0;
> +
> +	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)
> +{
> +	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
> +	int rv;
> +

You probably ought to handle O_NONBLOCK here.  (Same problem on BT, too.)

> +	rv = wait_event_interruptible(kcs_bmc->queue,
> +				kcs_bmc->data_in_avail != 0);
> +	if (rv < 0)
> +		return -ERESTARTSYS;
> +

This is a race condition for multiple users.

> +	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)
> +{
> +	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
> +	unsigned long flags;
> +
> +	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) {

If you don't modify kcs_phase here, you have a race condition.  You probably
need a KCS_WAIT_READ condition.  Also, the nomenclature of "read" and 
"write"
here is a little confusing, since your phases are from the host's point 
of view,
not this driver's point of view.  You might want to document that 
explicitly.

> +		kcs_bmc->data_out_idx = 1;
> +		kcs_bmc->data_out_len = count;
> +		kcs_outb(kcs_bmc, kcs_bmc->data_out[0], kcs_bmc->odr);
> +	}

So if you write and the data isn't ready, you just drop the data on the 
floor silently?
Probably not the best design.  You should probably return an error, as 
write can
only be called after read.

> +	spin_unlock_irqrestore(&kcs_bmc->lock, flags);
> +
> +	return count;
> +}
> +
> +static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd,
> +			  unsigned long arg)
> +{
> +	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
> +	long ret = 0;
> +
> +	switch (cmd) {
> +	case KCS_BMC_IOCTL_SET_ATN:
> +		kcs_set_atn(kcs_bmc);
> +		break;
> +
> +	case KCS_BMC_IOCTL_CLR_ATN:
> +		kcs_clear_atn(kcs_bmc);
> +		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)
> +{
> +	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
> +	unsigned long flags;
> +
> +	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 int kcs_bmc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct kcs_ioreg *ioreg;
> +	struct kcs_bmc *kcs_bmc;
> +	u32 chan, addr;
> +	int rc;
> +
> +	kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc), GFP_KERNEL);
> +	if (!kcs_bmc)
> +		return -ENOMEM;

Every error after this point will leak kcs_bmc.  I'd recommend a "goto 
out_err"
to handle this.

> +
> +	rc = of_property_read_u32(dev->of_node, "kcs_chan", &chan);
> +	if ((rc != 0) || (chan == 0 || chan > KCS_CHANNEL_MAX)) {
> +		dev_err(dev, "no valid 'kcs_chan' configured\n");
> +		return -ENODEV;
> +	}
> +
> +	rc = of_property_read_u32(dev->of_node, "kcs_addr", &addr);
> +	if (rc) {
> +		dev_err(dev, "no valid 'kcs_addr' configured\n");
> +		return -ENODEV;
> +	}
> +
> +	kcs_bmc->map = syscon_node_to_regmap(dev->parent->of_node);
> +	if (IS_ERR(kcs_bmc->map)) {
> +		dev_err(dev, "Couldn't get regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	dev_set_name(dev, "ipmi-kcs%u", chan);
> +
> +	spin_lock_init(&kcs_bmc->lock);
> +	kcs_bmc->chan = chan;

You need error checking on chan.

> +
> +	ioreg = &kcs_ioreg_map[chan - 1];
> +	kcs_bmc->idr  = ioreg->idr;
> +	kcs_bmc->odr  = ioreg->odr;
> +	kcs_bmc->str  = ioreg->str;
> +
> +	init_waitqueue_head(&kcs_bmc->queue);
> +	kcs_bmc->data_in  = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> +	kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> +	if (kcs_bmc->data_in == NULL || kcs_bmc->data_out == NULL) {
> +		dev_err(dev, "Failed to allocate data buffers\n");

You will leak memory if you fail to allocate data_out but do allocate 
data_in.

> +		return -ENOMEM;
> +	}
> +
> +	kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
> +	kcs_bmc->miscdev.name = dev_name(dev);
> +	kcs_bmc->miscdev.fops = &kcs_bmc_fops;
> +	rc = misc_register(&kcs_bmc->miscdev);
> +	if (rc) {
> +		dev_err(dev, "Unable to register device\n");
> +		return rc;
> +	}

After you call misc_register something can open the device and use it.
You need to do that after everything is enabled.

> +
> +	kcs_set_addr(kcs_bmc, addr);
> +	kcs_enable_channel(kcs_bmc, 1);
> +
> +	rc = kcs_bmc_config_irq(kcs_bmc, pdev);
> +	if (rc) {
> +		misc_deregister(&kcs_bmc->miscdev);
> +		return rc;
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, kcs_bmc);

This  should definitely be before you enable or register.  The drvdata
needs to be available in case an irq comes in, for instance.

> +
> +	dev_info(dev, "addr=0x%x, idr=0x%x, odr=0x%x, str=0x%x\n",
> +		addr, kcs_bmc->idr, kcs_bmc->odr, kcs_bmc->str);
> +
> +	return 0;
> +}
> +
> +static int kcs_bmc_remove(struct platform_device *pdev)
> +{
> +	struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
> +
> +	misc_deregister(&kcs_bmc->miscdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id kcs_bmc_match[] = {
> +	{ .compatible = "aspeed,ast2400-kcs-bmc" },
> +	{ .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 v2");
> +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..d1550d3
> --- /dev/null
> +++ b/include/uapi/linux/kcs-bmc.h
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2015-2018, 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_SET_ATN        _IO(__KCS_BMC_IOCTL_MAGIC, 1)
> +#define KCS_BMC_IOCTL_CLR_ATN        _IO(__KCS_BMC_IOCTL_MAGIC, 2)
> +#define KCS_BMC_IOCTL_FORCE_ABORT    _IO(__KCS_BMC_IOCTL_MAGIC, 3)
> +
> +#endif /* _UAPI_LINUX_KCS_BMC_H */

On 01/16/2018 02:59 PM, Corey Minyard wrote:
> On 01/16/2018 05:43 AM, Haiyue Wang wrote:
>> The KCS (Keyboard Controller Style) interface is used to perform in-band
>> IPMI communication between a server host and its BMC (BaseBoard 
>> Management
>> Controllers).
>>
>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and 
>> AST2500)
>> as a character device. Such SOCs are commonly used as BMCs and this 
>> driver
>> implements the BMC side of the KCS interface.
>
> I thought we were going to unify the BMC ioctl interface?  My 
> preference would be to
> create a file named include/uapi/linux/ipmi-bmc.h and add the following:
>
> #define __IPMI_BMC_IOCTL_MAGIC    0xb1
> #define IPMI_BMC_IOCTL_SMS_SET_ATN    _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>
> to make it the same as BT.  Then in bt-bmc.h, set BT_BMC_IOCTL_SMS_ATN to
> IPMI_BMC_IOCTL_SMS_SET_ATN.  Then add the KCS ioctls in ipmi-bmc.h and
> use that.  That way we stay backward compatible but we are unified.
>
> Since more KCS interfaces may come around, can you make the name more
> specific?  (I made this same error on bt-bmc,c, it should probably be 
> renamed.)
>
> More comments inline, as I'll go ahead and review this.
>
>> Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com>
>> ---
>>   .../devicetree/bindings/ipmi/aspeed-kcs-bmc.txt    |  26 +
>>   drivers/char/ipmi/Kconfig                          |   9 +
>>   drivers/char/ipmi/Makefile                         |   1 +
>>   drivers/char/ipmi/kcs-bmc.c                        | 744 
>> +++++++++++++++++++++
>>   include/uapi/linux/kcs-bmc.h                       |  14 +
>>   5 files changed, 794 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
>>   create mode 100644 drivers/char/ipmi/kcs-bmc.c
>>   create mode 100644 include/uapi/linux/kcs-bmc.h
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt 
>> b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
>> new file mode 100644
>> index 0000000..dd0c73d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
>> @@ -0,0 +1,26 @@
>> +* Aspeed KCS (Keyboard Controller Style) IPMI interface
>> +
>> +The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs
>> +(BaseBoard Management Controllers) and the KCS interface can be
>> +used to perform in-band IPMI communication with their host.
>> +
>> +Required properties:
>> +- compatible : should be one of
>> +    "aspeed,ast2400-kcs-bmc"
>> +    "aspeed,ast2500-kcs-bmc"
>> +- interrupts : interrupt generated by the controller
>> +- kcs_chan : The LPC channel number in the controller
>> +- kcs_addr : The host CPU IO map address
>> +
>> +
>> +Example:
>> +
>> +    kcs3: kcs3@0 {
>> +        compatible = "aspeed,ast2500-kcs-bmc";
>> +        reg = <0x0 0x80>;
>> +        interrupts = <8>;
>> +        kcs_chan = <3>;
>> +        kcs_addr = <0xCA2>;
>> +        status = "okay";
>> +    };
>> +
>> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
>> index 3544abc..36132f8 100644
>> --- a/drivers/char/ipmi/Kconfig
>> +++ b/drivers/char/ipmi/Kconfig
>> @@ -104,3 +104,12 @@ 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
>> +    select REGMAP_MMIO
>> +    tristate "KCS IPMI bmc driver"
>> +    help
>> +      Provides a driver for the KCS (Keyboard Controller Style) IPMI
>> +      interface found on Aspeed SOCs (AST2400 and 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 33b899f..f217bae 100644
>> --- a/drivers/char/ipmi/Makefile
>> +++ b/drivers/char/ipmi/Makefile
>> @@ -22,3 +22,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..d6eab0b
>> --- /dev/null
>> +++ b/drivers/char/ipmi/kcs-bmc.c
>> @@ -0,0 +1,744 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2015-2018, Intel Corporation.
>> +
>> +#include <linux/atomic.h>
>> +#include <linux/errno.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kcs-bmc.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/slab.h>
>> +#include <linux/timer.h>
>> +
>> +#define KCS_MSG_BUFSIZ      1024
>> +#define KCS_CHANNEL_MAX     4
>> +
>> +/*
>> + * 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
>
> It would be nicer to make the above an enum.
>
>> +
>> +/* Abort Phase */
>> +#define ABORT_PHASE_ERROR1      0x01
>> +#define ABORT_PHASE_ERROR2      0x02
>
> Can the above just be folded into two separate phases in kcs_phase?
> That would be a little cleaner.
>
>
>> +
>> +/* 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             BIT(3)
>> +#define KCS_STR_SMS_ATN             BIT(2)
>> +#define KCS_STR_IBF                 BIT(1)
>> +#define KCS_STR_OBF                 BIT(0)
>> +
>> +
>> +/* mapped to lpc-bmc@0 IO space */
>> +#define LPC_HICR0            0x000
>> +#define     LPC_HICR0_LPC3E          BIT(7)
>> +#define     LPC_HICR0_LPC2E          BIT(6)
>> +#define     LPC_HICR0_LPC1E          BIT(5)
>> +#define LPC_HICR2            0x008
>> +#define     LPC_HICR2_IBFIF3         BIT(3)
>> +#define     LPC_HICR2_IBFIF2         BIT(2)
>> +#define     LPC_HICR2_IBFIF1         BIT(1)
>> +#define LPC_HICR4            0x010
>> +#define     LPC_HICR4_LADR12AS       BIT(7)
>> +#define     LPC_HICR4_KCSENBL        BIT(2)
>> +#define LPC_LADR3H           0x014
>> +#define LPC_LADR3L           0x018
>> +#define LPC_LADR12H          0x01C
>> +#define LPC_LADR12L          0x020
>> +#define LPC_IDR1             0x024
>> +#define LPC_IDR2             0x028
>> +#define LPC_IDR3             0x02C
>> +#define LPC_ODR1             0x030
>> +#define LPC_ODR2             0x034
>> +#define LPC_ODR3             0x038
>> +#define LPC_STR1             0x03C
>> +#define LPC_STR2             0x040
>> +#define LPC_STR3             0x044
>> +
>> +/* mapped to lpc-host@80 IO space */
>> +#define LPC_HICRB            0x080
>> +#define     LPC_HICRB_IBFIF4         BIT(1)
>> +#define     LPC_HICRB_LPC4E          BIT(0)
>> +#define LPC_LADR4            0x090
>> +#define LPC_IDR4             0x094
>> +#define LPC_ODR4             0x098
>> +#define LPC_STR4             0x09C
>> +
>> +
>> +/* IPMI 2.0 - 9.5, KCS Interface Registers */
>> +struct kcs_ioreg {
>> +    u32 idr; /* Input Data Register */
>> +    u32 odr; /* Output Data Register */
>> +    u32 str; /* Status Register */
>> +};
>> +
>> +static const struct kcs_ioreg kcs_ioreg_map[KCS_CHANNEL_MAX] = {
>> +    { .idr = LPC_IDR1, .odr = LPC_ODR1, .str = LPC_STR1 },
>> +    { .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
>> +    { .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
>> +    { .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
>> +};

One more thing...  Why aren't the above values being set in the device tree?
Passing in a channel (and address) seems inflexible.  Kind of goes 
against the
philosophy of device trees.

-corey

>>
>> +
>> +struct kcs_bmc {
>> +    struct regmap *map;
>> +    spinlock_t     lock;
>
> This lock is only used in threads, as far as I can tell.  Couldn't it 
> just be a normal mutex?
> But more on this later.
>
>> +
>> +    u32 chan;
>> +    int running;
>> +
>> +    u32 idr;
>> +    u32 odr;
>> +    u32 str;
>> +
>> +    int kcs_phase;
>> +    u8  abort_phase;
>> +    u8  kcs_error;
>> +
>> +    wait_queue_head_t queue;
>> +    int  data_in_avail;
>
> data_in_avail should be a bool.
>
> You set data_in_avail after the data is ready, but you don't have a 
> barrier.  You
> have similar problems with kcs_phase.
>
> In fact, the locking in the driver doesn't seem quite correct.  If 
> this ever ran on
> an SMP system, it is likely to not work correctly.  You can assume 
> that the interrupt
> runs exclusively, but you cannot assume that the data operations are 
> available in
> order on another processor that handles a subsequent interrupt.
>
> The easiest way to fix this would be to add the spin lock around the 
> case statement
> in the irq handler and add them in the poll and read functions (you 
> would need to
> leave it as a spinlock in that case).  That would provide the proper 
> barriers assuming
> you put them in the right place (checking data_in_avail again inside 
> the spinlock
> in the read case, for instance).
>
>> +    int  data_in_idx;
>> +    u8  *data_in;
>> +
>> +    int  data_out_idx;
>> +    int  data_out_len;
>> +    u8  *data_out;
>> +
>> +    struct miscdevice miscdev;
>> +};
>> +
>> +static u8 kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
>> +{
>> +    u32 val = 0;
>> +    int rc;
>> +
>> +    rc = regmap_read(kcs_bmc->map, reg, &val);
>> +    WARN(rc != 0, "regmap_read() 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, reg, data);
>> +    WARN(rc != 0, "regmap_write() failed: %d\n", rc);
>> +}
>> +
>> +static inline void kcs_set_state(struct kcs_bmc *kcs_bmc, u8 state)
>> +{
>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_STATE_MASK,
>> +            KCS_STR_STATE(state));
>> +}
>> +
>> +static inline void kcs_set_atn(struct kcs_bmc *kcs_bmc)
>> +{
>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
>> +            KCS_STR_SMS_ATN);
>> +}
>> +
>> +static inline void kcs_clear_atn(struct kcs_bmc *kcs_bmc)
>> +{
>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
>> +            0);
>> +}
>> +
>> +/*
>> + * 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
>> + */
>> +static void kcs_set_addr(struct kcs_bmc *kcs_bmc, u16 addr)
>> +{
>> +    switch (kcs_bmc->chan) {
>> +    case 1:
>> +        regmap_update_bits(kcs_bmc->map, LPC_HICR4,
>> +                LPC_HICR4_LADR12AS, 0);
>> +        regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
>> +        regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
>> +        break;
>> +
>> +    case 2:
>> +        regmap_update_bits(kcs_bmc->map, LPC_HICR4,
>> +                LPC_HICR4_LADR12AS, LPC_HICR4_LADR12AS);
>> +        regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
>> +        regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
>> +        break;
>> +
>> +    case 3:
>> +        regmap_write(kcs_bmc->map, LPC_LADR3H, addr >> 8);
>> +        regmap_write(kcs_bmc->map, LPC_LADR3L, addr & 0xFF);
>> +        break;
>> +
>> +    case 4:
>> +        regmap_write(kcs_bmc->map, LPC_LADR4, ((addr + 1) << 16) |
>> +            addr);
>> +        break;
>> +
>> +    default:
>
> Shouldn't you return an error here?
>
>> +        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, 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);
>> +        }
>
> The above shouldn't have {}, did you run this through checkpatch?
>
>> +        break;
>> +
>> +    default:
>
> Error here, too?
>
>> +        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:
>
> This is identical to the default.  And the default should never 
> happen, anyway.
> If the default happens you have a software bug and need to report it.
>
>> +        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);
>
> Wouldn't you want to read the command before you write the OBF?
>
>> +    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);
>
> You don't set data_in_avail() to zero here?
>
>> +}
>> +
>> +static irqreturn_t kcs_bmc_irq(int irq, void *arg)
>> +{
>> +    struct kcs_bmc *kcs_bmc = arg;
>> +    u32 sts;
>> +
>> +    if (regmap_read(kcs_bmc->map, kcs_bmc->str, &sts) != 0)
>> +        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 irq;
>> +
>> +    irq = platform_get_irq(pdev, 0);
>> +    if (irq < 0)
>> +        return irq;
>> +
>> +    return devm_request_irq(dev, irq, kcs_bmc_irq, IRQF_SHARED,
>> +            dev_name(dev), kcs_bmc);
>> +}
>> +
>> +
>> +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)
>> +{
>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>> +    unsigned long flags;
>> +
>> +    if (kcs_bmc->running)
>> +        return -EBUSY;
>> +
>
> The above is a race, it needs to be done inside the lock.
>
>> + 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);
>
> What happens if the interface is not in a state where it can send a 
> message?
> The release code doesn't block until anything is done, so the 
> interface might
> not be in a place where you can use it.  I think the init code handles 
> it from
> that side, but the release side is not handled.
>
> Also, if release gets called, wouldn't you want to call 
> kcs_force_abort() here
> or in release()? That would be the equivalent of the BMC getting reset.
>
>> +
>> +    return 0;
>> +}
>> +
>> +static unsigned int kcs_bmc_poll(struct file *filp, poll_table *wait)
>> +{
>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>> +    unsigned int mask = 0;
>> +
>> +    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)
>> +{
>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>> +    int rv;
>> +
>
> You probably ought to handle O_NONBLOCK here.  (Same problem on BT, too.)
>
>> +    rv = wait_event_interruptible(kcs_bmc->queue,
>> +                kcs_bmc->data_in_avail != 0);
>> +    if (rv < 0)
>> +        return -ERESTARTSYS;
>> +
>
> This is a race condition for multiple users.
>
>> +    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)
>> +{
>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>> +    unsigned long flags;
>> +
>> +    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) {
>
> If you don't modify kcs_phase here, you have a race condition. You 
> probably
> need a KCS_WAIT_READ condition.  Also, the nomenclature of "read" and 
> "write"
> here is a little confusing, since your phases are from the host's 
> point of view,
> not this driver's point of view.  You might want to document that 
> explicitly.
>
>> +        kcs_bmc->data_out_idx = 1;
>> +        kcs_bmc->data_out_len = count;
>> +        kcs_outb(kcs_bmc, kcs_bmc->data_out[0], kcs_bmc->odr);
>> +    }
>
> So if you write and the data isn't ready, you just drop the data on 
> the floor silently?
> Probably not the best design.  You should probably return an error, as 
> write can
> only be called after read.
>
>> + spin_unlock_irqrestore(&kcs_bmc->lock, flags);
>> +
>> +    return count;
>> +}
>> +
>> +static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd,
>> +              unsigned long arg)
>> +{
>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>> +    long ret = 0;
>> +
>> +    switch (cmd) {
>> +    case KCS_BMC_IOCTL_SET_ATN:
>> +        kcs_set_atn(kcs_bmc);
>> +        break;
>> +
>> +    case KCS_BMC_IOCTL_CLR_ATN:
>> +        kcs_clear_atn(kcs_bmc);
>> +        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)
>> +{
>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>> +    unsigned long flags;
>> +
>> +    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 int kcs_bmc_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    const struct kcs_ioreg *ioreg;
>> +    struct kcs_bmc *kcs_bmc;
>> +    u32 chan, addr;
>> +    int rc;
>> +
>> +    kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc), GFP_KERNEL);
>> +    if (!kcs_bmc)
>> +        return -ENOMEM;
>
> Every error after this point will leak kcs_bmc.  I'd recommend a "goto 
> out_err"
> to handle this.
>
>> +
>> +    rc = of_property_read_u32(dev->of_node, "kcs_chan", &chan);
>> +    if ((rc != 0) || (chan == 0 || chan > KCS_CHANNEL_MAX)) {
>> +        dev_err(dev, "no valid 'kcs_chan' configured\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    rc = of_property_read_u32(dev->of_node, "kcs_addr", &addr);
>> +    if (rc) {
>> +        dev_err(dev, "no valid 'kcs_addr' configured\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    kcs_bmc->map = syscon_node_to_regmap(dev->parent->of_node);
>> +    if (IS_ERR(kcs_bmc->map)) {
>> +        dev_err(dev, "Couldn't get regmap\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    dev_set_name(dev, "ipmi-kcs%u", chan);
>> +
>> +    spin_lock_init(&kcs_bmc->lock);
>> +    kcs_bmc->chan = chan;
>
> You need error checking on chan.
>
>> +
>> +    ioreg = &kcs_ioreg_map[chan - 1];
>> +    kcs_bmc->idr  = ioreg->idr;
>> +    kcs_bmc->odr  = ioreg->odr;
>> +    kcs_bmc->str  = ioreg->str;
>> +
>> +    init_waitqueue_head(&kcs_bmc->queue);
>> +    kcs_bmc->data_in  = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
>> +    kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
>> +    if (kcs_bmc->data_in == NULL || kcs_bmc->data_out == NULL) {
>> +        dev_err(dev, "Failed to allocate data buffers\n");
>
> You will leak memory if you fail to allocate data_out but do allocate 
> data_in.
>
>> +        return -ENOMEM;
>> +    }
>> +
>> +    kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
>> +    kcs_bmc->miscdev.name = dev_name(dev);
>> +    kcs_bmc->miscdev.fops = &kcs_bmc_fops;
>> +    rc = misc_register(&kcs_bmc->miscdev);
>> +    if (rc) {
>> +        dev_err(dev, "Unable to register device\n");
>> +        return rc;
>> +    }
>
> After you call misc_register something can open the device and use it.
> You need to do that after everything is enabled.
>
>> +
>> +    kcs_set_addr(kcs_bmc, addr);
>> +    kcs_enable_channel(kcs_bmc, 1);
>> +
>> +    rc = kcs_bmc_config_irq(kcs_bmc, pdev);
>> +    if (rc) {
>> +        misc_deregister(&kcs_bmc->miscdev);
>> +        return rc;
>> +    }
>> +
>> +    dev_set_drvdata(&pdev->dev, kcs_bmc);
>
> This  should definitely be before you enable or register.  The drvdata
> needs to be available in case an irq comes in, for instance.
>
>> +
>> +    dev_info(dev, "addr=0x%x, idr=0x%x, odr=0x%x, str=0x%x\n",
>> +        addr, kcs_bmc->idr, kcs_bmc->odr, kcs_bmc->str);
>> +
>> +    return 0;
>> +}
>> +
>> +static int kcs_bmc_remove(struct platform_device *pdev)
>> +{
>> +    struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
>> +
>> +    misc_deregister(&kcs_bmc->miscdev);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct of_device_id kcs_bmc_match[] = {
>> +    { .compatible = "aspeed,ast2400-kcs-bmc" },
>> +    { .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 v2");
>> +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..d1550d3
>> --- /dev/null
>> +++ b/include/uapi/linux/kcs-bmc.h
>> @@ -0,0 +1,14 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2015-2018, 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_SET_ATN        _IO(__KCS_BMC_IOCTL_MAGIC, 1)
>> +#define KCS_BMC_IOCTL_CLR_ATN        _IO(__KCS_BMC_IOCTL_MAGIC, 2)
>> +#define KCS_BMC_IOCTL_FORCE_ABORT    _IO(__KCS_BMC_IOCTL_MAGIC, 3)
>> +
>> +#endif /* _UAPI_LINUX_KCS_BMC_H */
>
>

On Tue, Jan 16, 2018 at 2:59 PM, Corey Minyard <minyard@acm.org> wrote:
> On 01/16/2018 05:43 AM, Haiyue Wang wrote:
>>
>> The KCS (Keyboard Controller Style) interface is used to perform in-band
>> IPMI communication between a server host and its BMC (BaseBoard Management
>> Controllers).
>>
>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and AST2500)
>> as a character device. Such SOCs are commonly used as BMCs and this driver
>> implements the BMC side of the KCS interface.
>
>
> I thought we were going to unify the BMC ioctl interface?  My preference
> would be to
> create a file named include/uapi/linux/ipmi-bmc.h and add the following:
>
> #define __IPMI_BMC_IOCTL_MAGIC    0xb1
> #define IPMI_BMC_IOCTL_SMS_SET_ATN    _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>
> to make it the same as BT.  Then in bt-bmc.h, set BT_BMC_IOCTL_SMS_ATN to
> IPMI_BMC_IOCTL_SMS_SET_ATN.  Then add the KCS ioctls in ipmi-bmc.h and
> use that.  That way we stay backward compatible but we are unified.
>
> Since more KCS interfaces may come around, can you make the name more
> specific?  (I made this same error on bt-bmc,c, it should probably be
> renamed.)

Yes, we had a group of openbmc people get together recently and spoke
about this. Unfortunately Haiyue wasn't there (but other Intel BMC
people were).

We've got code coming from another BMC vendor who will use the same
userspace API. The intention is to unify ASPEED's KCS and BT, along
with Nuvoton's KCS and BT, as you outlined above.

Cheers,

Joel

On 2018-01-17 07:06, Joel Stanley wrote:
> On Tue, Jan 16, 2018 at 2:59 PM, Corey Minyard <minyard@acm.org> wrote:
>> On 01/16/2018 05:43 AM, Haiyue Wang wrote:
>>> The KCS (Keyboard Controller Style) interface is used to perform in-band
>>> IPMI communication between a server host and its BMC (BaseBoard Management
>>> Controllers).
>>>
>>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and AST2500)
>>> as a character device. Such SOCs are commonly used as BMCs and this driver
>>> implements the BMC side of the KCS interface.
>>
>> I thought we were going to unify the BMC ioctl interface?  My preference
>> would be to
>> create a file named include/uapi/linux/ipmi-bmc.h and add the following:
>>
>> #define __IPMI_BMC_IOCTL_MAGIC    0xb1
>> #define IPMI_BMC_IOCTL_SMS_SET_ATN    _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>>
>> to make it the same as BT.  Then in bt-bmc.h, set BT_BMC_IOCTL_SMS_ATN to
>> IPMI_BMC_IOCTL_SMS_SET_ATN.  Then add the KCS ioctls in ipmi-bmc.h and
>> use that.  That way we stay backward compatible but we are unified.
>>
>> Since more KCS interfaces may come around, can you make the name more
>> specific?  (I made this same error on bt-bmc,c, it should probably be
>> renamed.)
> Yes, we had a group of openbmc people get together recently and spoke
> about this. Unfortunately Haiyue wasn't there (but other Intel BMC
> people were).
>
> We've got code coming from another BMC vendor who will use the same
> userspace API. The intention is to unify ASPEED's KCS and BT, along
> with Nuvoton's KCS and BT, as you outlined above.
Great design for common BMC code, thanks Joel & Corey.

Then we need to divide the kcs-bmc.c into two files, one (still 
kcs-bmc.c) is for handling IPMI KCS state
according to IPMI 2.0 specification, and also handing device misc 
operations; another file is called such
as aspeed-kcs-bmc.c which handles ast2500 kcs controller. So that 
Nuvoton can define nvvoton-kcs-bmc.c
low level API, and call the IPMI KCS function in kcs-bmc.c ?

How about this idea ? If it makes sense, I will change the code for 
review in patch v2.

> Cheers,
>
> Joel

Sounds great for us (Nuvoton).

Avi.

On Wed, Jan 17, 2018 at 8:32 AM, Wang, Haiyue
<haiyue.wang@linux.intel.com> wrote:
>
>
> On 2018-01-17 07:06, Joel Stanley wrote:
>>
>> On Tue, Jan 16, 2018 at 2:59 PM, Corey Minyard <minyard@acm.org> wrote:
>>>
>>> On 01/16/2018 05:43 AM, Haiyue Wang wrote:
>>>>
>>>> The KCS (Keyboard Controller Style) interface is used to perform in-band
>>>> IPMI communication between a server host and its BMC (BaseBoard
>>>> Management
>>>> Controllers).
>>>>
>>>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and
>>>> AST2500)
>>>> as a character device. Such SOCs are commonly used as BMCs and this
>>>> driver
>>>> implements the BMC side of the KCS interface.
>>>
>>>
>>> I thought we were going to unify the BMC ioctl interface?  My preference
>>> would be to
>>> create a file named include/uapi/linux/ipmi-bmc.h and add the following:
>>>
>>> #define __IPMI_BMC_IOCTL_MAGIC    0xb1
>>> #define IPMI_BMC_IOCTL_SMS_SET_ATN    _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>>>
>>> to make it the same as BT.  Then in bt-bmc.h, set BT_BMC_IOCTL_SMS_ATN to
>>> IPMI_BMC_IOCTL_SMS_SET_ATN.  Then add the KCS ioctls in ipmi-bmc.h and
>>> use that.  That way we stay backward compatible but we are unified.
>>>
>>> Since more KCS interfaces may come around, can you make the name more
>>> specific?  (I made this same error on bt-bmc,c, it should probably be
>>> renamed.)
>>
>> Yes, we had a group of openbmc people get together recently and spoke
>> about this. Unfortunately Haiyue wasn't there (but other Intel BMC
>> people were).
>>
>> We've got code coming from another BMC vendor who will use the same
>> userspace API. The intention is to unify ASPEED's KCS and BT, along
>> with Nuvoton's KCS and BT, as you outlined above.
>
> Great design for common BMC code, thanks Joel & Corey.
>
> Then we need to divide the kcs-bmc.c into two files, one (still kcs-bmc.c)
> is for handling IPMI KCS state
> according to IPMI 2.0 specification, and also handing device misc
> operations; another file is called such
> as aspeed-kcs-bmc.c which handles ast2500 kcs controller. So that Nuvoton
> can define nvvoton-kcs-bmc.c
> low level API, and call the IPMI KCS function in kcs-bmc.c ?
>
> How about this idea ? If it makes sense, I will change the code for review
> in patch v2.
>
>> Cheers,
>>
>> Joel
>
>

On 2018-01-17 06:12, Corey Minyard wrote:
> On 01/16/2018 02:59 PM, Corey Minyard wrote:
>> On 01/16/2018 05:43 AM, Haiyue Wang wrote:
>>> The KCS (Keyboard Controller Style) interface is used to perform 
>>> in-band
>>> IPMI communication between a server host and its BMC (BaseBoard 
>>> Management
>>> Controllers).
>>>
>>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and 
>>> AST2500)
>>> as a character device. Such SOCs are commonly used as BMCs and this 
>>> driver
>>> implements the BMC side of the KCS interface.
>>
>> I thought we were going to unify the BMC ioctl interface?  My 
>> preference would be to
>> create a file named include/uapi/linux/ipmi-bmc.h and add the following:
>>
>> #define __IPMI_BMC_IOCTL_MAGIC    0xb1
>> #define IPMI_BMC_IOCTL_SMS_SET_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>>
>> to make it the same as BT.  Then in bt-bmc.h, set 
>> BT_BMC_IOCTL_SMS_ATN to
>> IPMI_BMC_IOCTL_SMS_SET_ATN.  Then add the KCS ioctls in ipmi-bmc.h and
>> use that.  That way we stay backward compatible but we are unified.
>>
>> Since more KCS interfaces may come around, can you make the name more
>> specific?  (I made this same error on bt-bmc,c, it should probably be 
>> renamed.)
>>
How about these IOCTL definitions ? Is it more specific ?

#define IPMI_BMC_IOCTL_SET_SMS_ATN    _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
#define IPMI_BMC_IOCTL_CLEAR_SMS_ATN  _IO(__IPMI_BMC_IOCTL_MAGIC, 0x01)
#define IPMI_BMC_IOCTL_FORCE_ABORT    _IO(__IPMI_BMC_IOCTL_MAGIC, 0x02)

>> More comments inline, as I'll go ahead and review this.
>>
>>> Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com>
>>> ---
>>>   .../devicetree/bindings/ipmi/aspeed-kcs-bmc.txt    |  26 +
>>>   drivers/char/ipmi/Kconfig                          |   9 +
>>>   drivers/char/ipmi/Makefile                         |   1 +
>>>   drivers/char/ipmi/kcs-bmc.c                        | 744 
>>> +++++++++++++++++++++
>>>   include/uapi/linux/kcs-bmc.h                       |  14 +
>>>   5 files changed, 794 insertions(+)
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
>>>   create mode 100644 drivers/char/ipmi/kcs-bmc.c
>>>   create mode 100644 include/uapi/linux/kcs-bmc.h
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt 
>>> b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
>>> new file mode 100644
>>> index 0000000..dd0c73d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
>>> @@ -0,0 +1,26 @@
>>> +* Aspeed KCS (Keyboard Controller Style) IPMI interface
>>> +
>>> +The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs
>>> +(BaseBoard Management Controllers) and the KCS interface can be
>>> +used to perform in-band IPMI communication with their host.
>>> +
>>> +Required properties:
>>> +- compatible : should be one of
>>> +    "aspeed,ast2400-kcs-bmc"
>>> +    "aspeed,ast2500-kcs-bmc"
>>> +- interrupts : interrupt generated by the controller
>>> +- kcs_chan : The LPC channel number in the controller
>>> +- kcs_addr : The host CPU IO map address
>>> +
>>> +
>>> +Example:
>>> +
>>> +    kcs3: kcs3@0 {
>>> +        compatible = "aspeed,ast2500-kcs-bmc";
>>> +        reg = <0x0 0x80>;
>>> +        interrupts = <8>;
>>> +        kcs_chan = <3>;
>>> +        kcs_addr = <0xCA2>;
>>> +        status = "okay";
>>> +    };
>>> +
>>> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
>>> index 3544abc..36132f8 100644
>>> --- a/drivers/char/ipmi/Kconfig
>>> +++ b/drivers/char/ipmi/Kconfig
>>> @@ -104,3 +104,12 @@ 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
>>> +    select REGMAP_MMIO
>>> +    tristate "KCS IPMI bmc driver"
>>> +    help
>>> +      Provides a driver for the KCS (Keyboard Controller Style) IPMI
>>> +      interface found on Aspeed SOCs (AST2400 and 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 33b899f..f217bae 100644
>>> --- a/drivers/char/ipmi/Makefile
>>> +++ b/drivers/char/ipmi/Makefile
>>> @@ -22,3 +22,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..d6eab0b
>>> --- /dev/null
>>> +++ b/drivers/char/ipmi/kcs-bmc.c
>>> @@ -0,0 +1,744 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +// Copyright (c) 2015-2018, Intel Corporation.
>>> +
>>> +#include <linux/atomic.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kcs-bmc.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/slab.h>
>>> +#include <linux/timer.h>
>>> +
>>> +#define KCS_MSG_BUFSIZ      1024
>>> +#define KCS_CHANNEL_MAX     4
>>> +
>>> +/*
>>> + * 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
>>
>> It would be nicer to make the above an enum.
>>
Done!
>>> +
>>> +/* Abort Phase */
>>> +#define ABORT_PHASE_ERROR1      0x01
>>> +#define ABORT_PHASE_ERROR2      0x02
>>
>> Can the above just be folded into two separate phases in kcs_phase?
>> That would be a little cleaner.
>>
Done, the code is truly cleaner, thanks! ;-)
>>
>>> +
>>> +/* 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             BIT(3)
>>> +#define KCS_STR_SMS_ATN             BIT(2)
>>> +#define KCS_STR_IBF                 BIT(1)
>>> +#define KCS_STR_OBF                 BIT(0)
>>> +
>>> +
>>> +/* mapped to lpc-bmc@0 IO space */
>>> +#define LPC_HICR0            0x000
>>> +#define     LPC_HICR0_LPC3E          BIT(7)
>>> +#define     LPC_HICR0_LPC2E          BIT(6)
>>> +#define     LPC_HICR0_LPC1E          BIT(5)
>>> +#define LPC_HICR2            0x008
>>> +#define     LPC_HICR2_IBFIF3         BIT(3)
>>> +#define     LPC_HICR2_IBFIF2         BIT(2)
>>> +#define     LPC_HICR2_IBFIF1         BIT(1)
>>> +#define LPC_HICR4            0x010
>>> +#define     LPC_HICR4_LADR12AS       BIT(7)
>>> +#define     LPC_HICR4_KCSENBL        BIT(2)
>>> +#define LPC_LADR3H           0x014
>>> +#define LPC_LADR3L           0x018
>>> +#define LPC_LADR12H          0x01C
>>> +#define LPC_LADR12L          0x020
>>> +#define LPC_IDR1             0x024
>>> +#define LPC_IDR2             0x028
>>> +#define LPC_IDR3             0x02C
>>> +#define LPC_ODR1             0x030
>>> +#define LPC_ODR2             0x034
>>> +#define LPC_ODR3             0x038
>>> +#define LPC_STR1             0x03C
>>> +#define LPC_STR2             0x040
>>> +#define LPC_STR3             0x044
>>> +
>>> +/* mapped to lpc-host@80 IO space */
>>> +#define LPC_HICRB            0x080
>>> +#define     LPC_HICRB_IBFIF4         BIT(1)
>>> +#define     LPC_HICRB_LPC4E          BIT(0)
>>> +#define LPC_LADR4            0x090
>>> +#define LPC_IDR4             0x094
>>> +#define LPC_ODR4             0x098
>>> +#define LPC_STR4             0x09C
>>> +
>>> +
>>> +/* IPMI 2.0 - 9.5, KCS Interface Registers */
>>> +struct kcs_ioreg {
>>> +    u32 idr; /* Input Data Register */
>>> +    u32 odr; /* Output Data Register */
>>> +    u32 str; /* Status Register */
>>> +};
>>> +
>>> +static const struct kcs_ioreg kcs_ioreg_map[KCS_CHANNEL_MAX] = {
>>> +    { .idr = LPC_IDR1, .odr = LPC_ODR1, .str = LPC_STR1 },
>>> +    { .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
>>> +    { .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
>>> +    { .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
>>> +};
>
> One more thing...  Why aren't the above values being set in the device 
> tree?
> Passing in a channel (and address) seems inflexible.  Kind of goes 
> against the
> philosophy of device trees.
>
> -corey
>
Change it by referring to Joel's suggestion, and defining IDR/ODR/STR 
registers together with other
registers for LPC KCS, looks like this made code be more easily maintained.

https://lists.ozlabs.org/pipermail/openbmc/2017-December/010095.html

>>>
>>> +
>>> +struct kcs_bmc {
>>> +    struct regmap *map;
>>> +    spinlock_t     lock;
>>
>> This lock is only used in threads, as far as I can tell. Couldn't it 
>> just be a normal mutex?
>> But more on this later.
>>
I missed this lock using in KCS ISR function, for AST2500 is single core 
CPU. The critical data such as
data_in_avail is shared between ISR and user thread, spinlock_t related 
API should be the right one ?
especially for SMP ?

static irqreturn_t kcs_bmc_irq(int irq, void *arg)
{
     ....
     spin_lock(&kcs_bmc->lock);  // <-- MISSED

     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);
         break;

     default:
         ret = IRQ_NONE;
         break;
     }

     spin_unlock(&kcs_bmc->lock); // <-- MISSED

     return ret;
}


>>> +
>>> +    u32 chan;
>>> +    int running;
>>> +
>>> +    u32 idr;
>>> +    u32 odr;
>>> +    u32 str;
>>> +
>>> +    int kcs_phase;
>>> +    u8  abort_phase;
>>> +    u8  kcs_error;
>>> +
>>> +    wait_queue_head_t queue;
>>> +    int  data_in_avail;
>>
>> data_in_avail should be a bool.
>>
>> You set data_in_avail after the data is ready, but you don't have a 
>> barrier.  You
>> have similar problems with kcs_phase.
>>
>> In fact, the locking in the driver doesn't seem quite correct. If 
>> this ever ran on
>> an SMP system, it is likely to not work correctly.  You can assume 
>> that the interrupt
>> runs exclusively, but you cannot assume that the data operations are 
>> available in
>> order on another processor that handles a subsequent interrupt.
>>
>> The easiest way to fix this would be to add the spin lock around the 
>> case statement
>> in the irq handler and add them in the poll and read functions (you 
>> would need to
>> leave it as a spinlock in that case).  That would provide the proper 
>> barriers assuming
>> you put them in the right place (checking data_in_avail again inside 
>> the spinlock
>> in the read case, for instance).
>>
Got it!
>>> +    int  data_in_idx;
>>> +    u8  *data_in;
>>> +
>>> +    int  data_out_idx;
>>> +    int  data_out_len;
>>> +    u8  *data_out;
>>> +
>>> +    struct miscdevice miscdev;
>>> +};
>>> +
>>> +static u8 kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
>>> +{
>>> +    u32 val = 0;
>>> +    int rc;
>>> +
>>> +    rc = regmap_read(kcs_bmc->map, reg, &val);
>>> +    WARN(rc != 0, "regmap_read() 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, reg, data);
>>> +    WARN(rc != 0, "regmap_write() failed: %d\n", rc);
>>> +}
>>> +
>>> +static inline void kcs_set_state(struct kcs_bmc *kcs_bmc, u8 state)
>>> +{
>>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_STATE_MASK,
>>> +            KCS_STR_STATE(state));
>>> +}
>>> +
>>> +static inline void kcs_set_atn(struct kcs_bmc *kcs_bmc)
>>> +{
>>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
>>> +            KCS_STR_SMS_ATN);
>>> +}
>>> +
>>> +static inline void kcs_clear_atn(struct kcs_bmc *kcs_bmc)
>>> +{
>>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
>>> +            0);
>>> +}
>>> +
>>> +/*
>>> + * 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
>>> + */
>>> +static void kcs_set_addr(struct kcs_bmc *kcs_bmc, u16 addr)
>>> +{
>>> +    switch (kcs_bmc->chan) {
>>> +    case 1:
>>> +        regmap_update_bits(kcs_bmc->map, LPC_HICR4,
>>> +                LPC_HICR4_LADR12AS, 0);
>>> +        regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
>>> +        regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
>>> +        break;
>>> +
>>> +    case 2:
>>> +        regmap_update_bits(kcs_bmc->map, LPC_HICR4,
>>> +                LPC_HICR4_LADR12AS, LPC_HICR4_LADR12AS);
>>> +        regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
>>> +        regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
>>> +        break;
>>> +
>>> +    case 3:
>>> +        regmap_write(kcs_bmc->map, LPC_LADR3H, addr >> 8);
>>> +        regmap_write(kcs_bmc->map, LPC_LADR3L, addr & 0xFF);
>>> +        break;
>>> +
>>> +    case 4:
>>> +        regmap_write(kcs_bmc->map, LPC_LADR4, ((addr + 1) << 16) |
>>> +            addr);
>>> +        break;
>>> +
>>> +    default:
>>
>> Shouldn't you return an error here?
>>
For I checked the channel number on kcs_bmc_probe, still need this kind 
of error handling ?

     rc = of_property_read_u32(dev->of_node, "kcs_chan", &chan);
     if ((rc != 0) || (chan == 0 || chan > KCS_CHANNEL_MAX)) {
         dev_err(dev, "no valid 'kcs_chan' configured\n");
         return -ENODEV;
     }

>>> +        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, 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);
>>> +        }
>>
>> The above shouldn't have {}, did you run this through checkpatch?
Yes, I run the checkpatch, no this warning. ;-) But well, I will remove 
the '{}'.
>>
>>> +        break;
>>> +
>>> +    default:
>>
>> Error here, too?
>>
For I checked the channel number on kcs_bmc_probe, still need this kind 
of error handling ?
>>> +        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:
>>
>> This is identical to the default.  And the default should never 
>> happen, anyway.
>> If the default happens you have a software bug and need to report it.
>>
For making code clean, I removed the KCS_PHASE_ERROR, just keep the 
'default' handling.
>>> +        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);
>>
>> Wouldn't you want to read the command before you write the OBF?
>>
The host SMS KCS state is : 'wait for IBF=0' --> 'wait for OBF=1' /  
'clear OBF', for racing
condition, BMC needs prepare OBF=1, then clear IBF. ;-)
>>> +    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);
>>
>> You don't set data_in_avail() to zero here?
>>
Done, added it.
>>> +}
>>> +
>>> +static irqreturn_t kcs_bmc_irq(int irq, void *arg)
>>> +{
>>> +    struct kcs_bmc *kcs_bmc = arg;
>>> +    u32 sts;
>>> +
>>> +    if (regmap_read(kcs_bmc->map, kcs_bmc->str, &sts) != 0)
>>> +        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 irq;
>>> +
>>> +    irq = platform_get_irq(pdev, 0);
>>> +    if (irq < 0)
>>> +        return irq;
>>> +
>>> +    return devm_request_irq(dev, irq, kcs_bmc_irq, IRQF_SHARED,
>>> +            dev_name(dev), kcs_bmc);
>>> +}
>>> +
>>> +
>>> +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)
>>> +{
>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>> +    unsigned long flags;
>>> +
>>> +    if (kcs_bmc->running)
>>> +        return -EBUSY;
>>> +
>>
>> The above is a race, it needs to be done inside the lock.
>>
Fixed!
>>> + 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);
>>
>> What happens if the interface is not in a state where it can send a 
>> message?
>> The release code doesn't block until anything is done, so the 
>> interface might
>> not be in a place where you can use it.  I think the init code 
>> handles it from
>> that side, but the release side is not handled.
>>
>> Also, if release gets called, wouldn't you want to call 
>> kcs_force_abort() here
>> or in release()? That would be the equivalent of the BMC getting reset.
>>
Yes, you are right. We meet this kind of bug that host is waiting BMC 
after it resets. So normally,
after the user ipmi stack is ready, it will call 
KCS_BMC_IOCTL_FORCE_ABORT, then the SMS can
get a right response.
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static unsigned int kcs_bmc_poll(struct file *filp, poll_table *wait)
>>> +{
>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>> +    unsigned int mask = 0;
>>> +
>>> +    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)
>>> +{
>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>> +    int rv;
>>> +
>>
>> You probably ought to handle O_NONBLOCK here.  (Same problem on BT, 
>> too.)
>>
Got it, will add this.
>>> +    rv = wait_event_interruptible(kcs_bmc->queue,
>>> +                kcs_bmc->data_in_avail != 0);
>>> +    if (rv < 0)
>>> +        return -ERESTARTSYS;
>>> +
>>
>> This is a race condition for multiple users.
>>
Got it, will fix this.
>>> +    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)
>>> +{
>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>> +    unsigned long flags;
>>> +
>>> +    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) {
>>
>> If you don't modify kcs_phase here, you have a race condition. You 
>> probably
>> need a KCS_WAIT_READ condition.  Also, the nomenclature of "read" and 
>> "write"
>> here is a little confusing, since your phases are from the host's 
>> point of view,
>> not this driver's point of view.  You might want to document that 
>> explicitly.
>>
The race condition means that the user MAY write the duplicated response ?
Will write some comments for these phase definition.
>>> +        kcs_bmc->data_out_idx = 1;
>>> +        kcs_bmc->data_out_len = count;
>>> +        kcs_outb(kcs_bmc, kcs_bmc->data_out[0], kcs_bmc->odr);
>>> +    }
>>
>> So if you write and the data isn't ready, you just drop the data on 
>> the floor silently?
>> Probably not the best design.  You should probably return an error, 
>> as write can
>> only be called after read.
>>
Yes, you are right, will return the right error code instead. ;-)
>>> + spin_unlock_irqrestore(&kcs_bmc->lock, flags);
>>> +
>>> +    return count;
>>> +}
>>> +
>>> +static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd,
>>> +              unsigned long arg)
>>> +{
>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>> +    long ret = 0;
>>> +
>>> +    switch (cmd) {
>>> +    case KCS_BMC_IOCTL_SET_ATN:
>>> +        kcs_set_atn(kcs_bmc);
>>> +        break;
>>> +
>>> +    case KCS_BMC_IOCTL_CLR_ATN:
>>> +        kcs_clear_atn(kcs_bmc);
>>> +        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)
>>> +{
>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>> +    unsigned long flags;
>>> +
>>> +    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 int kcs_bmc_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    const struct kcs_ioreg *ioreg;
>>> +    struct kcs_bmc *kcs_bmc;
>>> +    u32 chan, addr;
>>> +    int rc;
>>> +
>>> +    kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc), GFP_KERNEL);
>>> +    if (!kcs_bmc)
>>> +        return -ENOMEM;
>>
>> Every error after this point will leak kcs_bmc.  I'd recommend a 
>> "goto out_err"
>> to handle this.
>>
It is 'devm_xxx' API, it will be released automatically by the dev 
framework. It also can auto-release
the irq, so that probe function can be designed clearly. This is the 
first time I know about this. ;-)
>>> +
>>> +    rc = of_property_read_u32(dev->of_node, "kcs_chan", &chan);
>>> +    if ((rc != 0) || (chan == 0 || chan > KCS_CHANNEL_MAX)) {
>>> +        dev_err(dev, "no valid 'kcs_chan' configured\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    rc = of_property_read_u32(dev->of_node, "kcs_addr", &addr);
>>> +    if (rc) {
>>> +        dev_err(dev, "no valid 'kcs_addr' configured\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    kcs_bmc->map = syscon_node_to_regmap(dev->parent->of_node);
>>> +    if (IS_ERR(kcs_bmc->map)) {
>>> +        dev_err(dev, "Couldn't get regmap\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    dev_set_name(dev, "ipmi-kcs%u", chan);
>>> +
>>> +    spin_lock_init(&kcs_bmc->lock);
>>> +    kcs_bmc->chan = chan;
>>
>> You need error checking on chan.
>>
Has been checked above : if ((rc != 0) || (chan == 0 || chan > 
KCS_CHANNEL_MAX)) {
>>> +
>>> +    ioreg = &kcs_ioreg_map[chan - 1];
>>> +    kcs_bmc->idr  = ioreg->idr;
>>> +    kcs_bmc->odr  = ioreg->odr;
>>> +    kcs_bmc->str  = ioreg->str;
>>> +
>>> +    init_waitqueue_head(&kcs_bmc->queue);
>>> +    kcs_bmc->data_in  = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
>>> +    kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
>>> +    if (kcs_bmc->data_in == NULL || kcs_bmc->data_out == NULL) {
>>> +        dev_err(dev, "Failed to allocate data buffers\n");
>>
>> You will leak memory if you fail to allocate data_out but do allocate 
>> data_in.
>>
It is 'devm_xxx' API, it will be released automatically by the dev 
framework.;-)
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
>>> +    kcs_bmc->miscdev.name = dev_name(dev);
>>> +    kcs_bmc->miscdev.fops = &kcs_bmc_fops;
>>> +    rc = misc_register(&kcs_bmc->miscdev);
>>> +    if (rc) {
>>> +        dev_err(dev, "Unable to register device\n");
>>> +        return rc;
>>> +    }
>>
>> After you call misc_register something can open the device and use it.
>> You need to do that after everything is enabled.
>>
Got it, will change the code order.
>>> +
>>> +    kcs_set_addr(kcs_bmc, addr);
>>> +    kcs_enable_channel(kcs_bmc, 1);
>>> +
>>> +    rc = kcs_bmc_config_irq(kcs_bmc, pdev);
>>> +    if (rc) {
>>> +        misc_deregister(&kcs_bmc->miscdev);
>>> +        return rc;
>>> +    }
>>> +
>>> +    dev_set_drvdata(&pdev->dev, kcs_bmc);
>>
>> This  should definitely be before you enable or register.  The drvdata
>> needs to be available in case an irq comes in, for instance.
>>
Got it, will change the code order.
>>> +
>>> +    dev_info(dev, "addr=0x%x, idr=0x%x, odr=0x%x, str=0x%x\n",
>>> +        addr, kcs_bmc->idr, kcs_bmc->odr, kcs_bmc->str);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int kcs_bmc_remove(struct platform_device *pdev)
>>> +{
>>> +    struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
>>> +
>>> +    misc_deregister(&kcs_bmc->miscdev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct of_device_id kcs_bmc_match[] = {
>>> +    { .compatible = "aspeed,ast2400-kcs-bmc" },
>>> +    { .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 v2");
>>> +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..d1550d3
>>> --- /dev/null
>>> +++ b/include/uapi/linux/kcs-bmc.h
>>> @@ -0,0 +1,14 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +// Copyright (c) 2015-2018, 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_SET_ATN _IO(__KCS_BMC_IOCTL_MAGIC, 1)
>>> +#define KCS_BMC_IOCTL_CLR_ATN _IO(__KCS_BMC_IOCTL_MAGIC, 2)
>>> +#define KCS_BMC_IOCTL_FORCE_ABORT _IO(__KCS_BMC_IOCTL_MAGIC, 3)
>>> +
>>> +#endif /* _UAPI_LINUX_KCS_BMC_H */
>>
>>
>

Thanks Avi, wait for your response when new patch is ready. :-)

BR,
Haiyue


On 2018-01-17 20:54, Avi Fishman wrote:
> Sounds great for us (Nuvoton).
>
> Avi.
> On Wed, Jan 17, 2018 at 8:32 AM, Wang, Haiyue
> <haiyue.wang@linux.intel.com> wrote:
>>
>> On 2018-01-17 07:06, Joel Stanley wrote:
>>> On Tue, Jan 16, 2018 at 2:59 PM, Corey Minyard <minyard@acm.org> wrote:
>>>> On 01/16/2018 05:43 AM, Haiyue Wang wrote:
>>>>> The KCS (Keyboard Controller Style) interface is used to perform in-band
>>>>> IPMI communication between a server host and its BMC (BaseBoard
>>>>> Management
>>>>> Controllers).
>>>>>
>>>>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and
>>>>> AST2500)
>>>>> as a character device. Such SOCs are commonly used as BMCs and this
>>>>> driver
>>>>> implements the BMC side of the KCS interface.
>>>>
>>>> I thought we were going to unify the BMC ioctl interface?  My preference
>>>> would be to
>>>> create a file named include/uapi/linux/ipmi-bmc.h and add the following:
>>>>
>>>> #define __IPMI_BMC_IOCTL_MAGIC    0xb1
>>>> #define IPMI_BMC_IOCTL_SMS_SET_ATN    _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>>>>
>>>> to make it the same as BT.  Then in bt-bmc.h, set BT_BMC_IOCTL_SMS_ATN to
>>>> IPMI_BMC_IOCTL_SMS_SET_ATN.  Then add the KCS ioctls in ipmi-bmc.h and
>>>> use that.  That way we stay backward compatible but we are unified.
>>>>
>>>> Since more KCS interfaces may come around, can you make the name more
>>>> specific?  (I made this same error on bt-bmc,c, it should probably be
>>>> renamed.)
>>> Yes, we had a group of openbmc people get together recently and spoke
>>> about this. Unfortunately Haiyue wasn't there (but other Intel BMC
>>> people were).
>>>
>>> We've got code coming from another BMC vendor who will use the same
>>> userspace API. The intention is to unify ASPEED's KCS and BT, along
>>> with Nuvoton's KCS and BT, as you outlined above.
>> Great design for common BMC code, thanks Joel & Corey.
>>
>> Then we need to divide the kcs-bmc.c into two files, one (still kcs-bmc.c)
>> is for handling IPMI KCS state
>> according to IPMI 2.0 specification, and also handing device misc
>> operations; another file is called such
>> as aspeed-kcs-bmc.c which handles ast2500 kcs controller. So that Nuvoton
>> can define nvvoton-kcs-bmc.c
>> low level API, and call the IPMI KCS function in kcs-bmc.c ?
>>
>> How about this idea ? If it makes sense, I will change the code for review
>> in patch v2.
>>
>>> Cheers,
>>>
>>> Joel
>>

On 01/17/2018 08:31 AM, Wang, Haiyue wrote:
>
>
> On 2018-01-17 06:12, Corey Minyard wrote:
>> On 01/16/2018 02:59 PM, Corey Minyard wrote:
>>> On 01/16/2018 05:43 AM, Haiyue Wang wrote:
>>>> The KCS (Keyboard Controller Style) interface is used to perform 
>>>> in-band
>>>> IPMI communication between a server host and its BMC (BaseBoard 
>>>> Management
>>>> Controllers).
>>>>
>>>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and 
>>>> AST2500)
>>>> as a character device. Such SOCs are commonly used as BMCs and this 
>>>> driver
>>>> implements the BMC side of the KCS interface.
>>>
>>> I thought we were going to unify the BMC ioctl interface?  My 
>>> preference would be to
>>> create a file named include/uapi/linux/ipmi-bmc.h and add the 
>>> following:
>>>
>>> #define __IPMI_BMC_IOCTL_MAGIC    0xb1
>>> #define IPMI_BMC_IOCTL_SMS_SET_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>>>
>>> to make it the same as BT.  Then in bt-bmc.h, set 
>>> BT_BMC_IOCTL_SMS_ATN to
>>> IPMI_BMC_IOCTL_SMS_SET_ATN.  Then add the KCS ioctls in ipmi-bmc.h and
>>> use that.  That way we stay backward compatible but we are unified.
>>>
>>> Since more KCS interfaces may come around, can you make the name more
>>> specific?  (I made this same error on bt-bmc,c, it should probably 
>>> be renamed.)
>>>
> How about these IOCTL definitions ? Is it more specific ?
>
> #define IPMI_BMC_IOCTL_SET_SMS_ATN    _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
> #define IPMI_BMC_IOCTL_CLEAR_SMS_ATN  _IO(__IPMI_BMC_IOCTL_MAGIC, 0x01)
> #define IPMI_BMC_IOCTL_FORCE_ABORT    _IO(__IPMI_BMC_IOCTL_MAGIC, 0x02)
>

Those look good to me.  If you could do the switchover to ipmi-bmc.h in 
a separate
patch, that would be cleaner.  Then add the clear atn and force abort 
ioctls in the
patch to add the new driver.

Sound good?

-corey

>>> More comments inline, as I'll go ahead and review this.
>>>
>>>> Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com>
>>>> ---
>>>>   .../devicetree/bindings/ipmi/aspeed-kcs-bmc.txt    |  26 +
>>>>   drivers/char/ipmi/Kconfig                          |   9 +
>>>>   drivers/char/ipmi/Makefile                         |   1 +
>>>>   drivers/char/ipmi/kcs-bmc.c                        | 744 
>>>> +++++++++++++++++++++
>>>>   include/uapi/linux/kcs-bmc.h                       |  14 +
>>>>   5 files changed, 794 insertions(+)
>>>>   create mode 100644 
>>>> Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
>>>>   create mode 100644 drivers/char/ipmi/kcs-bmc.c
>>>>   create mode 100644 include/uapi/linux/kcs-bmc.h
>>>>
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt 
>>>> b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
>>>> new file mode 100644
>>>> index 0000000..dd0c73d
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
>>>> @@ -0,0 +1,26 @@
>>>> +* Aspeed KCS (Keyboard Controller Style) IPMI interface
>>>> +
>>>> +The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs
>>>> +(BaseBoard Management Controllers) and the KCS interface can be
>>>> +used to perform in-band IPMI communication with their host.
>>>> +
>>>> +Required properties:
>>>> +- compatible : should be one of
>>>> +    "aspeed,ast2400-kcs-bmc"
>>>> +    "aspeed,ast2500-kcs-bmc"
>>>> +- interrupts : interrupt generated by the controller
>>>> +- kcs_chan : The LPC channel number in the controller
>>>> +- kcs_addr : The host CPU IO map address
>>>> +
>>>> +
>>>> +Example:
>>>> +
>>>> +    kcs3: kcs3@0 {
>>>> +        compatible = "aspeed,ast2500-kcs-bmc";
>>>> +        reg = <0x0 0x80>;
>>>> +        interrupts = <8>;
>>>> +        kcs_chan = <3>;
>>>> +        kcs_addr = <0xCA2>;
>>>> +        status = "okay";
>>>> +    };
>>>> +
>>>> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
>>>> index 3544abc..36132f8 100644
>>>> --- a/drivers/char/ipmi/Kconfig
>>>> +++ b/drivers/char/ipmi/Kconfig
>>>> @@ -104,3 +104,12 @@ 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
>>>> +    select REGMAP_MMIO
>>>> +    tristate "KCS IPMI bmc driver"
>>>> +    help
>>>> +      Provides a driver for the KCS (Keyboard Controller Style) IPMI
>>>> +      interface found on Aspeed SOCs (AST2400 and 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 33b899f..f217bae 100644
>>>> --- a/drivers/char/ipmi/Makefile
>>>> +++ b/drivers/char/ipmi/Makefile
>>>> @@ -22,3 +22,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..d6eab0b
>>>> --- /dev/null
>>>> +++ b/drivers/char/ipmi/kcs-bmc.c
>>>> @@ -0,0 +1,744 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +// Copyright (c) 2015-2018, Intel Corporation.
>>>> +
>>>> +#include <linux/atomic.h>
>>>> +#include <linux/errno.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/kcs-bmc.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/slab.h>
>>>> +#include <linux/timer.h>
>>>> +
>>>> +#define KCS_MSG_BUFSIZ      1024
>>>> +#define KCS_CHANNEL_MAX     4
>>>> +
>>>> +/*
>>>> + * 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
>>>
>>> It would be nicer to make the above an enum.
>>>
> Done!
>>>> +
>>>> +/* Abort Phase */
>>>> +#define ABORT_PHASE_ERROR1      0x01
>>>> +#define ABORT_PHASE_ERROR2      0x02
>>>
>>> Can the above just be folded into two separate phases in kcs_phase?
>>> That would be a little cleaner.
>>>
> Done, the code is truly cleaner, thanks! ;-)
>>>
>>>> +
>>>> +/* 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             BIT(3)
>>>> +#define KCS_STR_SMS_ATN             BIT(2)
>>>> +#define KCS_STR_IBF                 BIT(1)
>>>> +#define KCS_STR_OBF                 BIT(0)
>>>> +
>>>> +
>>>> +/* mapped to lpc-bmc@0 IO space */
>>>> +#define LPC_HICR0            0x000
>>>> +#define     LPC_HICR0_LPC3E          BIT(7)
>>>> +#define     LPC_HICR0_LPC2E          BIT(6)
>>>> +#define     LPC_HICR0_LPC1E          BIT(5)
>>>> +#define LPC_HICR2            0x008
>>>> +#define     LPC_HICR2_IBFIF3         BIT(3)
>>>> +#define     LPC_HICR2_IBFIF2         BIT(2)
>>>> +#define     LPC_HICR2_IBFIF1         BIT(1)
>>>> +#define LPC_HICR4            0x010
>>>> +#define     LPC_HICR4_LADR12AS       BIT(7)
>>>> +#define     LPC_HICR4_KCSENBL        BIT(2)
>>>> +#define LPC_LADR3H           0x014
>>>> +#define LPC_LADR3L           0x018
>>>> +#define LPC_LADR12H          0x01C
>>>> +#define LPC_LADR12L          0x020
>>>> +#define LPC_IDR1             0x024
>>>> +#define LPC_IDR2             0x028
>>>> +#define LPC_IDR3             0x02C
>>>> +#define LPC_ODR1             0x030
>>>> +#define LPC_ODR2             0x034
>>>> +#define LPC_ODR3             0x038
>>>> +#define LPC_STR1             0x03C
>>>> +#define LPC_STR2             0x040
>>>> +#define LPC_STR3             0x044
>>>> +
>>>> +/* mapped to lpc-host@80 IO space */
>>>> +#define LPC_HICRB            0x080
>>>> +#define     LPC_HICRB_IBFIF4         BIT(1)
>>>> +#define     LPC_HICRB_LPC4E          BIT(0)
>>>> +#define LPC_LADR4            0x090
>>>> +#define LPC_IDR4             0x094
>>>> +#define LPC_ODR4             0x098
>>>> +#define LPC_STR4             0x09C
>>>> +
>>>> +
>>>> +/* IPMI 2.0 - 9.5, KCS Interface Registers */
>>>> +struct kcs_ioreg {
>>>> +    u32 idr; /* Input Data Register */
>>>> +    u32 odr; /* Output Data Register */
>>>> +    u32 str; /* Status Register */
>>>> +};
>>>> +
>>>> +static const struct kcs_ioreg kcs_ioreg_map[KCS_CHANNEL_MAX] = {
>>>> +    { .idr = LPC_IDR1, .odr = LPC_ODR1, .str = LPC_STR1 },
>>>> +    { .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
>>>> +    { .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
>>>> +    { .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
>>>> +};
>>
>> One more thing...  Why aren't the above values being set in the 
>> device tree?
>> Passing in a channel (and address) seems inflexible.  Kind of goes 
>> against the
>> philosophy of device trees.
>>
>> -corey
>>
> Change it by referring to Joel's suggestion, and defining IDR/ODR/STR 
> registers together with other
> registers for LPC KCS, looks like this made code be more easily 
> maintained.
>
> https://lists.ozlabs.org/pipermail/openbmc/2017-December/010095.html
>
>>>>
>>>> +
>>>> +struct kcs_bmc {
>>>> +    struct regmap *map;
>>>> +    spinlock_t     lock;
>>>
>>> This lock is only used in threads, as far as I can tell. Couldn't it 
>>> just be a normal mutex?
>>> But more on this later.
>>>
> I missed this lock using in KCS ISR function, for AST2500 is single 
> core CPU. The critical data such as
> data_in_avail is shared between ISR and user thread, spinlock_t 
> related API should be the right one ?
> especially for SMP ?
>
> static irqreturn_t kcs_bmc_irq(int irq, void *arg)
> {
>     ....
>     spin_lock(&kcs_bmc->lock);  // <-- MISSED
>
>     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);
>         break;
>
>     default:
>         ret = IRQ_NONE;
>         break;
>     }
>
>     spin_unlock(&kcs_bmc->lock); // <-- MISSED
>
>     return ret;
> }
>
>
>>>> +
>>>> +    u32 chan;
>>>> +    int running;
>>>> +
>>>> +    u32 idr;
>>>> +    u32 odr;
>>>> +    u32 str;
>>>> +
>>>> +    int kcs_phase;
>>>> +    u8  abort_phase;
>>>> +    u8  kcs_error;
>>>> +
>>>> +    wait_queue_head_t queue;
>>>> +    int  data_in_avail;
>>>
>>> data_in_avail should be a bool.
>>>
>>> You set data_in_avail after the data is ready, but you don't have a 
>>> barrier.  You
>>> have similar problems with kcs_phase.
>>>
>>> In fact, the locking in the driver doesn't seem quite correct. If 
>>> this ever ran on
>>> an SMP system, it is likely to not work correctly.  You can assume 
>>> that the interrupt
>>> runs exclusively, but you cannot assume that the data operations are 
>>> available in
>>> order on another processor that handles a subsequent interrupt.
>>>
>>> The easiest way to fix this would be to add the spin lock around the 
>>> case statement
>>> in the irq handler and add them in the poll and read functions (you 
>>> would need to
>>> leave it as a spinlock in that case).  That would provide the proper 
>>> barriers assuming
>>> you put them in the right place (checking data_in_avail again inside 
>>> the spinlock
>>> in the read case, for instance).
>>>
> Got it!
>>>> +    int  data_in_idx;
>>>> +    u8  *data_in;
>>>> +
>>>> +    int  data_out_idx;
>>>> +    int  data_out_len;
>>>> +    u8  *data_out;
>>>> +
>>>> +    struct miscdevice miscdev;
>>>> +};
>>>> +
>>>> +static u8 kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
>>>> +{
>>>> +    u32 val = 0;
>>>> +    int rc;
>>>> +
>>>> +    rc = regmap_read(kcs_bmc->map, reg, &val);
>>>> +    WARN(rc != 0, "regmap_read() 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, reg, data);
>>>> +    WARN(rc != 0, "regmap_write() failed: %d\n", rc);
>>>> +}
>>>> +
>>>> +static inline void kcs_set_state(struct kcs_bmc *kcs_bmc, u8 state)
>>>> +{
>>>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, 
>>>> KCS_STR_STATE_MASK,
>>>> +            KCS_STR_STATE(state));
>>>> +}
>>>> +
>>>> +static inline void kcs_set_atn(struct kcs_bmc *kcs_bmc)
>>>> +{
>>>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
>>>> +            KCS_STR_SMS_ATN);
>>>> +}
>>>> +
>>>> +static inline void kcs_clear_atn(struct kcs_bmc *kcs_bmc)
>>>> +{
>>>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
>>>> +            0);
>>>> +}
>>>> +
>>>> +/*
>>>> + * 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
>>>> + */
>>>> +static void kcs_set_addr(struct kcs_bmc *kcs_bmc, u16 addr)
>>>> +{
>>>> +    switch (kcs_bmc->chan) {
>>>> +    case 1:
>>>> +        regmap_update_bits(kcs_bmc->map, LPC_HICR4,
>>>> +                LPC_HICR4_LADR12AS, 0);
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
>>>> +        break;
>>>> +
>>>> +    case 2:
>>>> +        regmap_update_bits(kcs_bmc->map, LPC_HICR4,
>>>> +                LPC_HICR4_LADR12AS, LPC_HICR4_LADR12AS);
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
>>>> +        break;
>>>> +
>>>> +    case 3:
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR3H, addr >> 8);
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR3L, addr & 0xFF);
>>>> +        break;
>>>> +
>>>> +    case 4:
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR4, ((addr + 1) << 16) |
>>>> +            addr);
>>>> +        break;
>>>> +
>>>> +    default:
>>>
>>> Shouldn't you return an error here?
>>>
> For I checked the channel number on kcs_bmc_probe, still need this 
> kind of error handling ?
>
>     rc = of_property_read_u32(dev->of_node, "kcs_chan", &chan);
>     if ((rc != 0) || (chan == 0 || chan > KCS_CHANNEL_MAX)) {
>         dev_err(dev, "no valid 'kcs_chan' configured\n");
>         return -ENODEV;
>     }
>
>>>> +        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, 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);
>>>> +        }
>>>
>>> The above shouldn't have {}, did you run this through checkpatch?
> Yes, I run the checkpatch, no this warning. ;-) But well, I will 
> remove the '{}'.
>>>
>>>> +        break;
>>>> +
>>>> +    default:
>>>
>>> Error here, too?
>>>
> For I checked the channel number on kcs_bmc_probe, still need this 
> kind of error handling ?
>>>> +        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:
>>>
>>> This is identical to the default.  And the default should never 
>>> happen, anyway.
>>> If the default happens you have a software bug and need to report it.
>>>
> For making code clean, I removed the KCS_PHASE_ERROR, just keep the 
> 'default' handling.
>>>> +        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);
>>>
>>> Wouldn't you want to read the command before you write the OBF?
>>>
> The host SMS KCS state is : 'wait for IBF=0' --> 'wait for OBF=1' /  
> 'clear OBF', for racing
> condition, BMC needs prepare OBF=1, then clear IBF. ;-)
>>>> +    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);
>>>
>>> You don't set data_in_avail() to zero here?
>>>
> Done, added it.
>>>> +}
>>>> +
>>>> +static irqreturn_t kcs_bmc_irq(int irq, void *arg)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = arg;
>>>> +    u32 sts;
>>>> +
>>>> +    if (regmap_read(kcs_bmc->map, kcs_bmc->str, &sts) != 0)
>>>> +        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 irq;
>>>> +
>>>> +    irq = platform_get_irq(pdev, 0);
>>>> +    if (irq < 0)
>>>> +        return irq;
>>>> +
>>>> +    return devm_request_irq(dev, irq, kcs_bmc_irq, IRQF_SHARED,
>>>> +            dev_name(dev), kcs_bmc);
>>>> +}
>>>> +
>>>> +
>>>> +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)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    unsigned long flags;
>>>> +
>>>> +    if (kcs_bmc->running)
>>>> +        return -EBUSY;
>>>> +
>>>
>>> The above is a race, it needs to be done inside the lock.
>>>
> Fixed!
>>>> + 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);
>>>
>>> What happens if the interface is not in a state where it can send a 
>>> message?
>>> The release code doesn't block until anything is done, so the 
>>> interface might
>>> not be in a place where you can use it.  I think the init code 
>>> handles it from
>>> that side, but the release side is not handled.
>>>
>>> Also, if release gets called, wouldn't you want to call 
>>> kcs_force_abort() here
>>> or in release()? That would be the equivalent of the BMC getting reset.
>>>
> Yes, you are right. We meet this kind of bug that host is waiting BMC 
> after it resets. So normally,
> after the user ipmi stack is ready, it will call 
> KCS_BMC_IOCTL_FORCE_ABORT, then the SMS can
> get a right response.
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static unsigned int kcs_bmc_poll(struct file *filp, poll_table *wait)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    unsigned int mask = 0;
>>>> +
>>>> +    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)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    int rv;
>>>> +
>>>
>>> You probably ought to handle O_NONBLOCK here.  (Same problem on BT, 
>>> too.)
>>>
> Got it, will add this.
>>>> +    rv = wait_event_interruptible(kcs_bmc->queue,
>>>> +                kcs_bmc->data_in_avail != 0);
>>>> +    if (rv < 0)
>>>> +        return -ERESTARTSYS;
>>>> +
>>>
>>> This is a race condition for multiple users.
>>>
> Got it, will fix this.
>>>> +    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)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    unsigned long flags;
>>>> +
>>>> +    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) {
>>>
>>> If you don't modify kcs_phase here, you have a race condition. You 
>>> probably
>>> need a KCS_WAIT_READ condition.  Also, the nomenclature of "read" 
>>> and "write"
>>> here is a little confusing, since your phases are from the host's 
>>> point of view,
>>> not this driver's point of view.  You might want to document that 
>>> explicitly.
>>>
> The race condition means that the user MAY write the duplicated 
> response ?
> Will write some comments for these phase definition.
>>>> +        kcs_bmc->data_out_idx = 1;
>>>> +        kcs_bmc->data_out_len = count;
>>>> +        kcs_outb(kcs_bmc, kcs_bmc->data_out[0], kcs_bmc->odr);
>>>> +    }
>>>
>>> So if you write and the data isn't ready, you just drop the data on 
>>> the floor silently?
>>> Probably not the best design.  You should probably return an error, 
>>> as write can
>>> only be called after read.
>>>
> Yes, you are right, will return the right error code instead. ;-)
>>>> + spin_unlock_irqrestore(&kcs_bmc->lock, flags);
>>>> +
>>>> +    return count;
>>>> +}
>>>> +
>>>> +static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd,
>>>> +              unsigned long arg)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    long ret = 0;
>>>> +
>>>> +    switch (cmd) {
>>>> +    case KCS_BMC_IOCTL_SET_ATN:
>>>> +        kcs_set_atn(kcs_bmc);
>>>> +        break;
>>>> +
>>>> +    case KCS_BMC_IOCTL_CLR_ATN:
>>>> +        kcs_clear_atn(kcs_bmc);
>>>> +        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)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    unsigned long flags;
>>>> +
>>>> +    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 int kcs_bmc_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct device *dev = &pdev->dev;
>>>> +    const struct kcs_ioreg *ioreg;
>>>> +    struct kcs_bmc *kcs_bmc;
>>>> +    u32 chan, addr;
>>>> +    int rc;
>>>> +
>>>> +    kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc), GFP_KERNEL);
>>>> +    if (!kcs_bmc)
>>>> +        return -ENOMEM;
>>>
>>> Every error after this point will leak kcs_bmc.  I'd recommend a 
>>> "goto out_err"
>>> to handle this.
>>>
> It is 'devm_xxx' API, it will be released automatically by the dev 
> framework. It also can auto-release
> the irq, so that probe function can be designed clearly. This is the 
> first time I know about this. ;-)
>>>> +
>>>> +    rc = of_property_read_u32(dev->of_node, "kcs_chan", &chan);
>>>> +    if ((rc != 0) || (chan == 0 || chan > KCS_CHANNEL_MAX)) {
>>>> +        dev_err(dev, "no valid 'kcs_chan' configured\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    rc = of_property_read_u32(dev->of_node, "kcs_addr", &addr);
>>>> +    if (rc) {
>>>> +        dev_err(dev, "no valid 'kcs_addr' configured\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    kcs_bmc->map = syscon_node_to_regmap(dev->parent->of_node);
>>>> +    if (IS_ERR(kcs_bmc->map)) {
>>>> +        dev_err(dev, "Couldn't get regmap\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    dev_set_name(dev, "ipmi-kcs%u", chan);
>>>> +
>>>> +    spin_lock_init(&kcs_bmc->lock);
>>>> +    kcs_bmc->chan = chan;
>>>
>>> You need error checking on chan.
>>>
> Has been checked above : if ((rc != 0) || (chan == 0 || chan > 
> KCS_CHANNEL_MAX)) {
>>>> +
>>>> +    ioreg = &kcs_ioreg_map[chan - 1];
>>>> +    kcs_bmc->idr  = ioreg->idr;
>>>> +    kcs_bmc->odr  = ioreg->odr;
>>>> +    kcs_bmc->str  = ioreg->str;
>>>> +
>>>> +    init_waitqueue_head(&kcs_bmc->queue);
>>>> +    kcs_bmc->data_in  = devm_kmalloc(dev, KCS_MSG_BUFSIZ, 
>>>> GFP_KERNEL);
>>>> +    kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, 
>>>> GFP_KERNEL);
>>>> +    if (kcs_bmc->data_in == NULL || kcs_bmc->data_out == NULL) {
>>>> +        dev_err(dev, "Failed to allocate data buffers\n");
>>>
>>> You will leak memory if you fail to allocate data_out but do 
>>> allocate data_in.
>>>
> It is 'devm_xxx' API, it will be released automatically by the dev 
> framework.;-)
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +
>>>> +    kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
>>>> +    kcs_bmc->miscdev.name = dev_name(dev);
>>>> +    kcs_bmc->miscdev.fops = &kcs_bmc_fops;
>>>> +    rc = misc_register(&kcs_bmc->miscdev);
>>>> +    if (rc) {
>>>> +        dev_err(dev, "Unable to register device\n");
>>>> +        return rc;
>>>> +    }
>>>
>>> After you call misc_register something can open the device and use it.
>>> You need to do that after everything is enabled.
>>>
> Got it, will change the code order.
>>>> +
>>>> +    kcs_set_addr(kcs_bmc, addr);
>>>> +    kcs_enable_channel(kcs_bmc, 1);
>>>> +
>>>> +    rc = kcs_bmc_config_irq(kcs_bmc, pdev);
>>>> +    if (rc) {
>>>> +        misc_deregister(&kcs_bmc->miscdev);
>>>> +        return rc;
>>>> +    }
>>>> +
>>>> +    dev_set_drvdata(&pdev->dev, kcs_bmc);
>>>
>>> This  should definitely be before you enable or register.  The drvdata
>>> needs to be available in case an irq comes in, for instance.
>>>
> Got it, will change the code order.
>>>> +
>>>> +    dev_info(dev, "addr=0x%x, idr=0x%x, odr=0x%x, str=0x%x\n",
>>>> +        addr, kcs_bmc->idr, kcs_bmc->odr, kcs_bmc->str);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int kcs_bmc_remove(struct platform_device *pdev)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
>>>> +
>>>> +    misc_deregister(&kcs_bmc->miscdev);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id kcs_bmc_match[] = {
>>>> +    { .compatible = "aspeed,ast2400-kcs-bmc" },
>>>> +    { .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 v2");
>>>> +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..d1550d3
>>>> --- /dev/null
>>>> +++ b/include/uapi/linux/kcs-bmc.h
>>>> @@ -0,0 +1,14 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +// Copyright (c) 2015-2018, 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_SET_ATN _IO(__KCS_BMC_IOCTL_MAGIC, 1)
>>>> +#define KCS_BMC_IOCTL_CLR_ATN _IO(__KCS_BMC_IOCTL_MAGIC, 2)
>>>> +#define KCS_BMC_IOCTL_FORCE_ABORT _IO(__KCS_BMC_IOCTL_MAGIC, 3)
>>>> +
>>>> +#endif /* _UAPI_LINUX_KCS_BMC_H */
>>>
>>>
>>
>

On 01/17/2018 08:31 AM, Wang, Haiyue wrote:
>
>

Snip...

>>>> +/* mapped to lpc-host@80 IO space */
>>>> +#define LPC_HICRB            0x080
>>>> +#define     LPC_HICRB_IBFIF4         BIT(1)
>>>> +#define     LPC_HICRB_LPC4E          BIT(0)
>>>> +#define LPC_LADR4            0x090
>>>> +#define LPC_IDR4             0x094
>>>> +#define LPC_ODR4             0x098
>>>> +#define LPC_STR4             0x09C
>>>> +
>>>> +
>>>> +/* IPMI 2.0 - 9.5, KCS Interface Registers */
>>>> +struct kcs_ioreg {
>>>> +    u32 idr; /* Input Data Register */
>>>> +    u32 odr; /* Output Data Register */
>>>> +    u32 str; /* Status Register */
>>>> +};
>>>> +
>>>> +static const struct kcs_ioreg kcs_ioreg_map[KCS_CHANNEL_MAX] = {
>>>> +    { .idr = LPC_IDR1, .odr = LPC_ODR1, .str = LPC_STR1 },
>>>> +    { .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
>>>> +    { .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
>>>> +    { .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
>>>> +};
>>
>> One more thing...  Why aren't the above values being set in the 
>> device tree?
>> Passing in a channel (and address) seems inflexible.  Kind of goes 
>> against the
>> philosophy of device trees.
>>
>> -corey
>>
> Change it by referring to Joel's suggestion, and defining IDR/ODR/STR 
> registers together with other
> registers for LPC KCS, looks like this made code be more easily 
> maintained.
>
> https://lists.ozlabs.org/pipermail/openbmc/2017-December/010095.html
>

Hmm, the code that you had there seemed ok to me (though you didn't 
check that you got three
registers).

I'm ok either way.  The idea of the device tree is that if someone has 
this same device,
they just add a device tree and it works without modifying the driver.  
If Joel doesn't
think it's worth it, then I guess it's ok.

>>>>
>>>> +
>>>> +struct kcs_bmc {
>>>> +    struct regmap *map;
>>>> +    spinlock_t     lock;
>>>
>>> This lock is only used in threads, as far as I can tell. Couldn't it 
>>> just be a normal mutex?
>>> But more on this later.
>>>
> I missed this lock using in KCS ISR function, for AST2500 is single 
> core CPU. The critical data such as
> data_in_avail is shared between ISR and user thread, spinlock_t 
> related API should be the right one ?
> especially for SMP ?
>

Sort of.  In the case below, you need to use spin_lock_irqsave(), you 
don't necessarily get
here with interrupts disabled.

In the ones called from user context, you should really use 
spin_lock_irq().  Interrupts
should always be on at that point, so it's better.

> static irqreturn_t kcs_bmc_irq(int irq, void *arg)
> {
>     ....
>     spin_lock(&kcs_bmc->lock);  // <-- MISSED
>
>     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);
>         break;
>
>     default:
>         ret = IRQ_NONE;
>         break;
>     }
>
>     spin_unlock(&kcs_bmc->lock); // <-- MISSED
>
>     return ret;
> }
>
>
>>>> +
>>>> +    u32 chan;
>>>> +    int running;
>>>> +
>>>> +    u32 idr;
>>>> +    u32 odr;
>>>> +    u32 str;
>>>> +
>>>> +    int kcs_phase;
>>>> +    u8  abort_phase;
>>>> +    u8  kcs_error;
>>>> +
>>>> +    wait_queue_head_t queue;
>>>> +    int  data_in_avail;
>>>
>>> data_in_avail should be a bool.
>>>
>>> You set data_in_avail after the data is ready, but you don't have a 
>>> barrier.  You
>>> have similar problems with kcs_phase.
>>>
>>> In fact, the locking in the driver doesn't seem quite correct. If 
>>> this ever ran on
>>> an SMP system, it is likely to not work correctly.  You can assume 
>>> that the interrupt
>>> runs exclusively, but you cannot assume that the data operations are 
>>> available in
>>> order on another processor that handles a subsequent interrupt.
>>>
>>> The easiest way to fix this would be to add the spin lock around the 
>>> case statement
>>> in the irq handler and add them in the poll and read functions (you 
>>> would need to
>>> leave it as a spinlock in that case).  That would provide the proper 
>>> barriers assuming
>>> you put them in the right place (checking data_in_avail again inside 
>>> the spinlock
>>> in the read case, for instance).
>>>
> Got it!
>>>> +    int  data_in_idx;
>>>> +    u8  *data_in;
>>>> +
>>>> +    int  data_out_idx;
>>>> +    int  data_out_len;
>>>> +    u8  *data_out;
>>>> +
>>>> +    struct miscdevice miscdev;
>>>> +};
>>>> +
>>>> +static u8 kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
>>>> +{
>>>> +    u32 val = 0;
>>>> +    int rc;
>>>> +
>>>> +    rc = regmap_read(kcs_bmc->map, reg, &val);
>>>> +    WARN(rc != 0, "regmap_read() 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, reg, data);
>>>> +    WARN(rc != 0, "regmap_write() failed: %d\n", rc);
>>>> +}
>>>> +
>>>> +static inline void kcs_set_state(struct kcs_bmc *kcs_bmc, u8 state)
>>>> +{
>>>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, 
>>>> KCS_STR_STATE_MASK,
>>>> +            KCS_STR_STATE(state));
>>>> +}
>>>> +
>>>> +static inline void kcs_set_atn(struct kcs_bmc *kcs_bmc)
>>>> +{
>>>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
>>>> +            KCS_STR_SMS_ATN);
>>>> +}
>>>> +
>>>> +static inline void kcs_clear_atn(struct kcs_bmc *kcs_bmc)
>>>> +{
>>>> +    regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
>>>> +            0);
>>>> +}
>>>> +
>>>> +/*
>>>> + * 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
>>>> + */
>>>> +static void kcs_set_addr(struct kcs_bmc *kcs_bmc, u16 addr)
>>>> +{
>>>> +    switch (kcs_bmc->chan) {
>>>> +    case 1:
>>>> +        regmap_update_bits(kcs_bmc->map, LPC_HICR4,
>>>> +                LPC_HICR4_LADR12AS, 0);
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
>>>> +        break;
>>>> +
>>>> +    case 2:
>>>> +        regmap_update_bits(kcs_bmc->map, LPC_HICR4,
>>>> +                LPC_HICR4_LADR12AS, LPC_HICR4_LADR12AS);
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
>>>> +        break;
>>>> +
>>>> +    case 3:
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR3H, addr >> 8);
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR3L, addr & 0xFF);
>>>> +        break;
>>>> +
>>>> +    case 4:
>>>> +        regmap_write(kcs_bmc->map, LPC_LADR4, ((addr + 1) << 16) |
>>>> +            addr);
>>>> +        break;
>>>> +
>>>> +    default:
>>>
>>> Shouldn't you return an error here?
>>>
> For I checked the channel number on kcs_bmc_probe, still need this 
> kind of error handling ?
>
>     rc = of_property_read_u32(dev->of_node, "kcs_chan", &chan);
>     if ((rc != 0) || (chan == 0 || chan > KCS_CHANNEL_MAX)) {
>         dev_err(dev, "no valid 'kcs_chan' configured\n");
>         return -ENODEV;
>     }
>

That's fine.

>>>> +        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, 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);
>>>> +        }
>>>
>>> The above shouldn't have {}, did you run this through checkpatch?
> Yes, I run the checkpatch, no this warning. ;-) But well, I will 
> remove the '{}'.
>>>
>>>> +        break;
>>>> +
>>>> +    default:
>>>
>>> Error here, too?
>>>
> For I checked the channel number on kcs_bmc_probe, still need this 
> kind of error handling ?

Just checking one place is fine :).

>>>> +        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:
>>>
>>> This is identical to the default.  And the default should never 
>>> happen, anyway.
>>> If the default happens you have a software bug and need to report it.
>>>
> For making code clean, I removed the KCS_PHASE_ERROR, just keep the 
> 'default' handling.
>>>> +        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);
>>>
>>> Wouldn't you want to read the command before you write the OBF?
>>>
> The host SMS KCS state is : 'wait for IBF=0' --> 'wait for OBF=1' /  
> 'clear OBF', for racing
> condition, BMC needs prepare OBF=1, then clear IBF. ;-)
>>>> +    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);
>>>
>>> You don't set data_in_avail() to zero here?
>>>
> Done, added it.
>>>> +}
>>>> +
>>>> +static irqreturn_t kcs_bmc_irq(int irq, void *arg)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = arg;
>>>> +    u32 sts;
>>>> +
>>>> +    if (regmap_read(kcs_bmc->map, kcs_bmc->str, &sts) != 0)
>>>> +        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 irq;
>>>> +
>>>> +    irq = platform_get_irq(pdev, 0);
>>>> +    if (irq < 0)
>>>> +        return irq;
>>>> +
>>>> +    return devm_request_irq(dev, irq, kcs_bmc_irq, IRQF_SHARED,
>>>> +            dev_name(dev), kcs_bmc);
>>>> +}
>>>> +
>>>> +
>>>> +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)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    unsigned long flags;
>>>> +
>>>> +    if (kcs_bmc->running)
>>>> +        return -EBUSY;
>>>> +
>>>
>>> The above is a race, it needs to be done inside the lock.
>>>
> Fixed!
>>>> + 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);
>>>
>>> What happens if the interface is not in a state where it can send a 
>>> message?
>>> The release code doesn't block until anything is done, so the 
>>> interface might
>>> not be in a place where you can use it.  I think the init code 
>>> handles it from
>>> that side, but the release side is not handled.
>>>
>>> Also, if release gets called, wouldn't you want to call 
>>> kcs_force_abort() here
>>> or in release()? That would be the equivalent of the BMC getting reset.
>>>
> Yes, you are right. We meet this kind of bug that host is waiting BMC 
> after it resets. So normally,
> after the user ipmi stack is ready, it will call 
> KCS_BMC_IOCTL_FORCE_ABORT, then the SMS can
> get a right response.
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static unsigned int kcs_bmc_poll(struct file *filp, poll_table *wait)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    unsigned int mask = 0;
>>>> +
>>>> +    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)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    int rv;
>>>> +
>>>
>>> You probably ought to handle O_NONBLOCK here.  (Same problem on BT, 
>>> too.)
>>>
> Got it, will add this.
>>>> +    rv = wait_event_interruptible(kcs_bmc->queue,
>>>> +                kcs_bmc->data_in_avail != 0);
>>>> +    if (rv < 0)
>>>> +        return -ERESTARTSYS;
>>>> +
>>>
>>> This is a race condition for multiple users.
>>>
> Got it, will fix this.
>>>> +    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)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    unsigned long flags;
>>>> +
>>>> +    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) {
>>>
>>> If you don't modify kcs_phase here, you have a race condition. You 
>>> probably
>>> need a KCS_WAIT_READ condition.  Also, the nomenclature of "read" 
>>> and "write"
>>> here is a little confusing, since your phases are from the host's 
>>> point of view,
>>> not this driver's point of view.  You might want to document that 
>>> explicitly.
>>>
> The race condition means that the user MAY write the duplicated 
> response ?

Not exactly.  Two threads can call this, and if it hasn't transitions 
from the read phase,
the data out will be overwritten.

> Will write some comments for these phase definition.
>>>> +        kcs_bmc->data_out_idx = 1;
>>>> +        kcs_bmc->data_out_len = count;
>>>> +        kcs_outb(kcs_bmc, kcs_bmc->data_out[0], kcs_bmc->odr);
>>>> +    }
>>>
>>> So if you write and the data isn't ready, you just drop the data on 
>>> the floor silently?
>>> Probably not the best design.  You should probably return an error, 
>>> as write can
>>> only be called after read.
>>>
> Yes, you are right, will return the right error code instead. ;-)
>>>> + spin_unlock_irqrestore(&kcs_bmc->lock, flags);
>>>> +
>>>> +    return count;
>>>> +}
>>>> +
>>>> +static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd,
>>>> +              unsigned long arg)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    long ret = 0;
>>>> +
>>>> +    switch (cmd) {
>>>> +    case KCS_BMC_IOCTL_SET_ATN:
>>>> +        kcs_set_atn(kcs_bmc);
>>>> +        break;
>>>> +
>>>> +    case KCS_BMC_IOCTL_CLR_ATN:
>>>> +        kcs_clear_atn(kcs_bmc);
>>>> +        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)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>>>> +    unsigned long flags;
>>>> +
>>>> +    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 int kcs_bmc_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct device *dev = &pdev->dev;
>>>> +    const struct kcs_ioreg *ioreg;
>>>> +    struct kcs_bmc *kcs_bmc;
>>>> +    u32 chan, addr;
>>>> +    int rc;
>>>> +
>>>> +    kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc), GFP_KERNEL);
>>>> +    if (!kcs_bmc)
>>>> +        return -ENOMEM;
>>>
>>> Every error after this point will leak kcs_bmc.  I'd recommend a 
>>> "goto out_err"
>>> to handle this.
>>>
> It is 'devm_xxx' API, it will be released automatically by the dev 
> framework. It also can auto-release
> the irq, so that probe function can be designed clearly. This is the 
> first time I know about this. ;-)

Oh, you are right, yes.  Sorry about that.

>>>> +
>>>> +    rc = of_property_read_u32(dev->of_node, "kcs_chan", &chan);
>>>> +    if ((rc != 0) || (chan == 0 || chan > KCS_CHANNEL_MAX)) {
>>>> +        dev_err(dev, "no valid 'kcs_chan' configured\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    rc = of_property_read_u32(dev->of_node, "kcs_addr", &addr);
>>>> +    if (rc) {
>>>> +        dev_err(dev, "no valid 'kcs_addr' configured\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    kcs_bmc->map = syscon_node_to_regmap(dev->parent->of_node);
>>>> +    if (IS_ERR(kcs_bmc->map)) {
>>>> +        dev_err(dev, "Couldn't get regmap\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    dev_set_name(dev, "ipmi-kcs%u", chan);
>>>> +
>>>> +    spin_lock_init(&kcs_bmc->lock);
>>>> +    kcs_bmc->chan = chan;
>>>
>>> You need error checking on chan.
>>>
> Has been checked above : if ((rc != 0) || (chan == 0 || chan > 
> KCS_CHANNEL_MAX)) {

Yeah, sorry I missed that.

-corey

>>>> +
>>>> +    ioreg = &kcs_ioreg_map[chan - 1];
>>>> +    kcs_bmc->idr  = ioreg->idr;
>>>> +    kcs_bmc->odr  = ioreg->odr;
>>>> +    kcs_bmc->str  = ioreg->str;
>>>> +
>>>> +    init_waitqueue_head(&kcs_bmc->queue);
>>>> +    kcs_bmc->data_in  = devm_kmalloc(dev, KCS_MSG_BUFSIZ, 
>>>> GFP_KERNEL);
>>>> +    kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, 
>>>> GFP_KERNEL);
>>>> +    if (kcs_bmc->data_in == NULL || kcs_bmc->data_out == NULL) {
>>>> +        dev_err(dev, "Failed to allocate data buffers\n");
>>>
>>> You will leak memory if you fail to allocate data_out but do 
>>> allocate data_in.
>>>
> It is 'devm_xxx' API, it will be released automatically by the dev 
> framework.;-)
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +
>>>> +    kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
>>>> +    kcs_bmc->miscdev.name = dev_name(dev);
>>>> +    kcs_bmc->miscdev.fops = &kcs_bmc_fops;
>>>> +    rc = misc_register(&kcs_bmc->miscdev);
>>>> +    if (rc) {
>>>> +        dev_err(dev, "Unable to register device\n");
>>>> +        return rc;
>>>> +    }
>>>
>>> After you call misc_register something can open the device and use it.
>>> You need to do that after everything is enabled.
>>>
> Got it, will change the code order.
>>>> +
>>>> +    kcs_set_addr(kcs_bmc, addr);
>>>> +    kcs_enable_channel(kcs_bmc, 1);
>>>> +
>>>> +    rc = kcs_bmc_config_irq(kcs_bmc, pdev);
>>>> +    if (rc) {
>>>> +        misc_deregister(&kcs_bmc->miscdev);
>>>> +        return rc;
>>>> +    }
>>>> +
>>>> +    dev_set_drvdata(&pdev->dev, kcs_bmc);
>>>
>>> This  should definitely be before you enable or register.  The drvdata
>>> needs to be available in case an irq comes in, for instance.
>>>
> Got it, will change the code order.
>>>> +
>>>> +    dev_info(dev, "addr=0x%x, idr=0x%x, odr=0x%x, str=0x%x\n",
>>>> +        addr, kcs_bmc->idr, kcs_bmc->odr, kcs_bmc->str);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int kcs_bmc_remove(struct platform_device *pdev)
>>>> +{
>>>> +    struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
>>>> +
>>>> +    misc_deregister(&kcs_bmc->miscdev);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id kcs_bmc_match[] = {
>>>> +    { .compatible = "aspeed,ast2400-kcs-bmc" },
>>>> +    { .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 v2");
>>>> +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..d1550d3
>>>> --- /dev/null
>>>> +++ b/include/uapi/linux/kcs-bmc.h
>>>> @@ -0,0 +1,14 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +// Copyright (c) 2015-2018, 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_SET_ATN _IO(__KCS_BMC_IOCTL_MAGIC, 1)
>>>> +#define KCS_BMC_IOCTL_CLR_ATN _IO(__KCS_BMC_IOCTL_MAGIC, 2)
>>>> +#define KCS_BMC_IOCTL_FORCE_ABORT _IO(__KCS_BMC_IOCTL_MAGIC, 3)
>>>> +
>>>> +#endif /* _UAPI_LINUX_KCS_BMC_H */
>>>
>>>
>>
>

On 2018-01-17 23:59, Corey Minyard wrote:
> On 01/17/2018 08:31 AM, Wang, Haiyue wrote:
>>
>>
>> On 2018-01-17 06:12, Corey Minyard wrote:
>>> On 01/16/2018 02:59 PM, Corey Minyard wrote:
>>>> On 01/16/2018 05:43 AM, Haiyue Wang wrote:
>>>>> The KCS (Keyboard Controller Style) interface is used to perform 
>>>>> in-band
>>>>> IPMI communication between a server host and its BMC (BaseBoard 
>>>>> Management
>>>>> Controllers).
>>>>>
>>>>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and 
>>>>> AST2500)
>>>>> as a character device. Such SOCs are commonly used as BMCs and 
>>>>> this driver
>>>>> implements the BMC side of the KCS interface.
>>>>
>>>> I thought we were going to unify the BMC ioctl interface? My 
>>>> preference would be to
>>>> create a file named include/uapi/linux/ipmi-bmc.h and add the 
>>>> following:
>>>>
>>>> #define __IPMI_BMC_IOCTL_MAGIC    0xb1
>>>> #define IPMI_BMC_IOCTL_SMS_SET_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>>>>
>>>> to make it the same as BT.  Then in bt-bmc.h, set 
>>>> BT_BMC_IOCTL_SMS_ATN to
>>>> IPMI_BMC_IOCTL_SMS_SET_ATN.  Then add the KCS ioctls in ipmi-bmc.h and
>>>> use that.  That way we stay backward compatible but we are unified.
>>>>
>>>> Since more KCS interfaces may come around, can you make the name more
>>>> specific?  (I made this same error on bt-bmc,c, it should probably 
>>>> be renamed.)
>>>>
>> How about these IOCTL definitions ? Is it more specific ?
>>
>> #define IPMI_BMC_IOCTL_SET_SMS_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>> #define IPMI_BMC_IOCTL_CLEAR_SMS_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x01)
>> #define IPMI_BMC_IOCTL_FORCE_ABORT _IO(__IPMI_BMC_IOCTL_MAGIC, 0x02)
>>
>
> Those look good to me.  If you could do the switchover to ipmi-bmc.h 
> in a separate
> patch, that would be cleaner.  Then add the clear atn and force abort 
> ioctls in the
> patch to add the new driver.
>
> Sound good?
>
> -corey
>
If I understood correctly, still use KCS_BMC_IOCTL_xxx in kcs_bmc.h 
currently, then add a
patch for ipmi-bmc.h, and modify the bt_bmc.h together. Right ?

Haiyue

On 2018-01-18 00:31, Corey Minyard wrote:
> On 01/17/2018 08:31 AM, Wang, Haiyue wrote:
>>
>>
>
> Snip...
>
>>>>>
>>>>> +
>>>>> +struct kcs_bmc {
>>>>> +    struct regmap *map;
>>>>> +    spinlock_t     lock;
>>>>
>>>> This lock is only used in threads, as far as I can tell. Couldn't 
>>>> it just be a normal mutex?
>>>> But more on this later.
>>>>
>> I missed this lock using in KCS ISR function, for AST2500 is single 
>> core CPU. The critical data such as
>> data_in_avail is shared between ISR and user thread, spinlock_t 
>> related API should be the right one ?
>> especially for SMP ?
>>
>
> Sort of.  In the case below, you need to use spin_lock_irqsave(), you 
> don't necessarily get
> here with interrupts disabled.
>
> In the ones called from user context, you should really use 
> spin_lock_irq().  Interrupts
> should always be on at that point, so it's better.
>
Understood, will change it with the right API call.
>> static irqreturn_t kcs_bmc_irq(int irq, void *arg)
>> {
>>     ....
>>     spin_lock(&kcs_bmc->lock);  // <-- MISSED
>>
>>     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);
>>         break;
>>
>>     default:
>>         ret = IRQ_NONE;
>>         break;
>>     }
>>
>>     spin_unlock(&kcs_bmc->lock); // <-- MISSED
>>
>>     return ret;
>> }
>>
>>
>
>>>>> + spin_lock_irqsave(&kcs_bmc->lock, flags);
>>>>> +    if (kcs_bmc->kcs_phase == KCS_PHASE_READ) {
>>>>
>>>> If you don't modify kcs_phase here, you have a race condition. You 
>>>> probably
>>>> need a KCS_WAIT_READ condition.  Also, the nomenclature of "read" 
>>>> and "write"
>>>> here is a little confusing, since your phases are from the host's 
>>>> point of view,
>>>> not this driver's point of view.  You might want to document that 
>>>> explicitly.
>>>>
>> The race condition means that the user MAY write the duplicated 
>> response ?
>
> Not exactly.  Two threads can call this, and if it hasn't transitions 
> from the read phase,
> the data out will be overwritten.
>
OK, will add new state KCS_WAIT_READ handling.

On 01/17/2018 06:16 PM, Wang, Haiyue wrote:
>
>
> On 2018-01-17 23:59, Corey Minyard wrote:
>> On 01/17/2018 08:31 AM, Wang, Haiyue wrote:
>>>
>>>
>>> On 2018-01-17 06:12, Corey Minyard wrote:
>>>> On 01/16/2018 02:59 PM, Corey Minyard wrote:
>>>>> On 01/16/2018 05:43 AM, Haiyue Wang wrote:
>>>>>> The KCS (Keyboard Controller Style) interface is used to perform 
>>>>>> in-band
>>>>>> IPMI communication between a server host and its BMC (BaseBoard 
>>>>>> Management
>>>>>> Controllers).
>>>>>>
>>>>>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and 
>>>>>> AST2500)
>>>>>> as a character device. Such SOCs are commonly used as BMCs and 
>>>>>> this driver
>>>>>> implements the BMC side of the KCS interface.
>>>>>
>>>>> I thought we were going to unify the BMC ioctl interface? My 
>>>>> preference would be to
>>>>> create a file named include/uapi/linux/ipmi-bmc.h and add the 
>>>>> following:
>>>>>
>>>>> #define __IPMI_BMC_IOCTL_MAGIC    0xb1
>>>>> #define IPMI_BMC_IOCTL_SMS_SET_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>>>>>
>>>>> to make it the same as BT.  Then in bt-bmc.h, set 
>>>>> BT_BMC_IOCTL_SMS_ATN to
>>>>> IPMI_BMC_IOCTL_SMS_SET_ATN.  Then add the KCS ioctls in ipmi-bmc.h 
>>>>> and
>>>>> use that.  That way we stay backward compatible but we are unified.
>>>>>
>>>>> Since more KCS interfaces may come around, can you make the name more
>>>>> specific?  (I made this same error on bt-bmc,c, it should probably 
>>>>> be renamed.)
>>>>>
>>> How about these IOCTL definitions ? Is it more specific ?
>>>
>>> #define IPMI_BMC_IOCTL_SET_SMS_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>>> #define IPMI_BMC_IOCTL_CLEAR_SMS_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x01)
>>> #define IPMI_BMC_IOCTL_FORCE_ABORT _IO(__IPMI_BMC_IOCTL_MAGIC, 0x02)
>>>
>>
>> Those look good to me.  If you could do the switchover to ipmi-bmc.h 
>> in a separate
>> patch, that would be cleaner.  Then add the clear atn and force abort 
>> ioctls in the
>> patch to add the new driver.
>>
>> Sound good?
>>
>> -corey
>>
> If I understood correctly, still use KCS_BMC_IOCTL_xxx in kcs_bmc.h 
> currently, then add a
> patch for ipmi-bmc.h, and modify the bt_bmc.h together. Right ?
>

No, not exactly.  Just add ipmi-bmc.h and put the ioctls you define 
above in it.  No need for
kcs_bmc.h at all.  We can then switch bt-bmc.c over to use the new 
ioctls later and remove
bt_bmc.h when all the software gets switched over.

-corey

> Haiyue

On 2018-01-18 10:58, Corey Minyard wrote:
> On 01/17/2018 06:16 PM, Wang, Haiyue wrote:
>>
>>
>> On 2018-01-17 23:59, Corey Minyard wrote:
>>> On 01/17/2018 08:31 AM, Wang, Haiyue wrote:
>>>>
>>>>
>>>> On 2018-01-17 06:12, Corey Minyard wrote:
>>>>> On 01/16/2018 02:59 PM, Corey Minyard wrote:
>>>>>> On 01/16/2018 05:43 AM, Haiyue Wang wrote:
>>>>>>> The KCS (Keyboard Controller Style) interface is used to perform 
>>>>>>> in-band
>>>>>>> IPMI communication between a server host and its BMC (BaseBoard 
>>>>>>> Management
>>>>>>> Controllers).
>>>>>>>
>>>>>>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 
>>>>>>> and AST2500)
>>>>>>> as a character device. Such SOCs are commonly used as BMCs and 
>>>>>>> this driver
>>>>>>> implements the BMC side of the KCS interface.
>>>>>>
>>>>>> I thought we were going to unify the BMC ioctl interface? My 
>>>>>> preference would be to
>>>>>> create a file named include/uapi/linux/ipmi-bmc.h and add the 
>>>>>> following:
>>>>>>
>>>>>> #define __IPMI_BMC_IOCTL_MAGIC    0xb1
>>>>>> #define IPMI_BMC_IOCTL_SMS_SET_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>>>>>>
>>>>>> to make it the same as BT.  Then in bt-bmc.h, set 
>>>>>> BT_BMC_IOCTL_SMS_ATN to
>>>>>> IPMI_BMC_IOCTL_SMS_SET_ATN.  Then add the KCS ioctls in 
>>>>>> ipmi-bmc.h and
>>>>>> use that.  That way we stay backward compatible but we are unified.
>>>>>>
>>>>>> Since more KCS interfaces may come around, can you make the name 
>>>>>> more
>>>>>> specific?  (I made this same error on bt-bmc,c, it should 
>>>>>> probably be renamed.)
>>>>>>
>>>> How about these IOCTL definitions ? Is it more specific ?
>>>>
>>>> #define IPMI_BMC_IOCTL_SET_SMS_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
>>>> #define IPMI_BMC_IOCTL_CLEAR_SMS_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x01)
>>>> #define IPMI_BMC_IOCTL_FORCE_ABORT _IO(__IPMI_BMC_IOCTL_MAGIC, 0x02)
>>>>
>>>
>>> Those look good to me.  If you could do the switchover to ipmi-bmc.h 
>>> in a separate
>>> patch, that would be cleaner.  Then add the clear atn and force 
>>> abort ioctls in the
>>> patch to add the new driver.
>>>
>>> Sound good?
>>>
>>> -corey
>>>
>> If I understood correctly, still use KCS_BMC_IOCTL_xxx in kcs_bmc.h 
>> currently, then add a
>> patch for ipmi-bmc.h, and modify the bt_bmc.h together. Right ?
>>
>
> No, not exactly.  Just add ipmi-bmc.h and put the ioctls you define 
> above in it.  No need for
> kcs_bmc.h at all.  We can then switch bt-bmc.c over to use the new 
> ioctls later and remove
> bt_bmc.h when all the software gets switched over.
>
Understood. Will use the new ioctls for kcs_bmc firstly.
> -corey
>
>> Haiyue
>
>