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

[Wang, 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