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

From: Wang, Haiyue
Date: Wed Jan 17 2018 - 01:32:44 EST




On 2018-01-17 07:06, Joel Stanley wrote:
On Tue, Jan 16, 2018 at 2:59 PM, Corey Minyard <minyard@xxxxxxx> 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