Re: [Openipmi-developer] [PATCH arm/aspeed/ast2500 v5 1/2] ipmi: add a KCS IPMI BMC driver

2018-02-02 Thread Corey Minyard 

On 02/01/2018 08:16 PM, Haiyue Wang wrote:

---
v4->v5
- Fix -Wdiscarded-qualifiers 'const' compile warning.
- Fix size_t printk compile error.

v3->v4
- Change to accept WRITE_START any time.

v2->v3

- Update the KCS phase state machine.
- Fix the race condition of read/write.

v1->v2

- Divide the driver into two parts, one handles the BMC KCS IPMI 2.0 state;
   the other handles the BMC KCS controller such as AST2500 IO accessing.
- Use the spin lock APIs to handle the device file operations and BMC chip
   IRQ inferface for accessing the same KCS BMC data structure.
- Enhanced the phases handling of the KCS BMC.
- Unified the IOCTL definition for IPMI BMC, it will be used by KCS and BT.


Provides a device driver for the KCS (Keyboard Controller Style)
IPMI interface which meets the requirement of the BMC (Baseboard
Management Controllers) side for handling the IPMI request from
host system software.


Ok, this is in my queue, it will go into next once 4.16-rc1 comes out, 
then into

4.16 if all goes well.

Thanks, for your patience and work on this.

-corey


Signed-off-by: Haiyue Wang 
---
  drivers/char/ipmi/Kconfig |   8 +
  drivers/char/ipmi/Makefile|   1 +
  drivers/char/ipmi/kcs_bmc.c   | 464 ++
  drivers/char/ipmi/kcs_bmc.h   | 106 ++
  include/uapi/linux/ipmi_bmc.h |  14 ++
  5 files changed, 593 insertions(+)
  create mode 100644 drivers/char/ipmi/kcs_bmc.c
  create mode 100644 drivers/char/ipmi/kcs_bmc.h
  create mode 100644 include/uapi/linux/ipmi_bmc.h

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 3544abc..aa9bcb1 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -96,6 +96,14 @@ config IPMI_POWEROFF
  
  endif # IPMI_HANDLER
  
+config IPMI_KCS_BMC

+   tristate 'IPMI KCS BMC Interface'
+   help
+ Provides a device driver for the KCS (Keyboard Controller Style)
+ IPMI interface which meets the requirement of the BMC (Baseboard
+ Management Controllers) side for handling the IPMI request from
+ host system software.
+
  config ASPEED_BT_IPMI_BMC
depends on ARCH_ASPEED || COMPILE_TEST
 depends on REGMAP && REGMAP_MMIO && MFD_SYSCON
diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
index 33b899f..2abccb3 100644
--- a/drivers/char/ipmi/Makefile
+++ b/drivers/char/ipmi/Makefile
@@ -21,4 +21,5 @@ obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o
  obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
  obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
  obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
+obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o
  obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
new file mode 100644
index 000..3a3498a
--- /dev/null
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -0,0 +1,464 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2015-2018, Intel Corporation.
+
+#define pr_fmt(fmt) "kcs-bmc: " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "kcs_bmc.h"
+
+#define KCS_MSG_BUFSIZ1000
+
+#define KCS_ZERO_DATA 0
+
+
+/* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */
+#define KCS_STATUS_STATE(state) (state << 6)
+#define KCS_STATUS_STATE_MASK   GENMASK(7, 6)
+#define KCS_STATUS_CMD_DAT  BIT(3)
+#define KCS_STATUS_SMS_ATN  BIT(2)
+#define KCS_STATUS_IBF  BIT(1)
+#define KCS_STATUS_OBF  BIT(0)
+
+/* IPMI 2.0 - Table 9-2, KCS Interface State Bits */
+enum kcs_states {
+   IDLE_STATE  = 0,
+   READ_STATE  = 1,
+   WRITE_STATE = 2,
+   ERROR_STATE = 3,
+};
+
+/* IPMI 2.0 - Table 9-3, KCS Interface Control Codes */
+#define KCS_CMD_GET_STATUS_ABORT  0x60
+#define KCS_CMD_WRITE_START   0x61
+#define KCS_CMD_WRITE_END 0x62
+#define KCS_CMD_READ_BYTE 0x68
+
+static inline u8 read_data(struct kcs_bmc *kcs_bmc)
+{
+   return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr);
+}
+
+static inline void write_data(struct kcs_bmc *kcs_bmc, u8 data)
+{
+   kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data);
+}
+
+static inline u8 read_status(struct kcs_bmc *kcs_bmc)
+{
+   return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.str);
+}
+
+static inline void write_status(struct kcs_bmc *kcs_bmc, u8 data)
+{
+   kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data);
+}
+
+static void update_status_bits(struct kcs_bmc *kcs_bmc, u8 mask, u8 val)
+{
+   u8 tmp = read_status(kcs_bmc);
+
+   tmp &= ~mask;
+   tmp |= val & mask;
+
+   write_status(kcs_bmc, tmp);
+}
+
+static inline void set_state(struct kcs_bmc *kcs_bmc, u8 state)
+{
+   update_status_bits(kcs_bmc, KCS_STATUS_STATE_MASK,
+   KCS_STATUS_STATE(state));
+}
+
+static void kcs_force_abort(struct kcs_bmc *kcs_bmc)
+{
+   set_state(kcs_bmc, ERROR_STATE);
+

Re: [Openipmi-developer] [PATCH arm/aspeed/ast2500 v4 1/2] ipmi: add a KCS IPMI BMC driver

2018-02-02 Thread Corey Minyard 

On 02/01/2018 07:33 PM, Wang, Haiyue wrote:



On 2018-02-02 09:10, Corey Minyard wrote:


I loaded this in, tried a compile on x86_64, and got the following:

In file included from ../drivers/char/ipmi/kcs_bmc.c:15:0:
../drivers/char/ipmi/kcs_bmc.h: In function ‘kcs_bmc_priv’:
../drivers/char/ipmi/kcs_bmc.h:100:9: warning: return discards 
‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]

  return kcs_bmc->priv;
 ^

-static inline void *kcs_bmc_priv(const struct kcs_bmc *kcs_bmc)
+static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)  <-- Can 
this fix error on x86_64 ?

{
    return kcs_bmc->priv;
}


That, or you can separately alloc the priv data and just make it a 
pointer.  That's more standard, but either way will work.


Where you not getting this warning on your compile?


In file included from ../include/linux/printk.h:7:0,
 from ../include/linux/kernel.h:14,
 from ../include/asm-generic/bug.h:18,
 from ../arch/x86/include/asm/bug.h:82,
 from ../include/linux/bug.h:5,
 from ../include/linux/io.h:23,
 from ../drivers/char/ipmi/kcs_bmc.c:7:
../drivers/char/ipmi/kcs_bmc.c: In function ‘kcs_bmc_read’:
../include/linux/kern_levels.h:5:18: warning: format ‘%u’ expects 
argument of type ‘unsigned int’, but argument 3 has type ‘size_t {aka 
long unsigned int}’ [-Wformat=]

 #define KERN_SOH "\001"  /* ASCII Start Of Header */
  ^
../include/linux/kern_levels.h:11:18: note: in expansion of macro 
‘KERN_SOH’

 #define KERN_ERR KERN_SOH "3" /* error conditions */
  ^
../include/linux/printk.h:301:9: note: in expansion of macro ‘KERN_ERR’
  printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
 ^
../drivers/char/ipmi/kcs_bmc.c:307:3: note: in expansion of macro 
‘pr_err’

   pr_err("channel=%u with too large data : %u\n",
   ^

https://elinux.org/Debugging_by_printing
However please*note*: always use/%zu/,/%zd/or/%zx/for 
printing/size_t/and/ssize_t/values. ssize_t and size_t are quite 
common values in the kernel, so please use the/%z/to avoid annoying 
compile warnings.
So I change it from "%u" to "%zu", it is passed on my arm-32 compile, 
is it OK on your X64 ?


Should be good.

Thanks,

-corey


pr_err("channel=%u with too large data : %zu\n",

In file included from ../drivers/char/ipmi/kcs_bmc_aspeed.c:20:0:
../drivers/char/ipmi/kcs_bmc.h: In function ‘kcs_bmc_priv’:
../drivers/char/ipmi/kcs_bmc.h:100:9: warning: return discards 
‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]

  return kcs_bmc->priv;
 ^

So that needs to be fixed before it goes in.

Also, since you are respinning, can you make ASPEED_KCS_IPMI_BMC 
select IPMI_KCS_BMC, like:


diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index bc2568a..d34f40e 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -99,16 +99,11 @@ config IPMI_POWEROFF
 endif # IPMI_HANDLER

 config IPMI_KCS_BMC
-   tristate 'IPMI KCS BMC Interface'
-   help
- Provides a device driver for the KCS (Keyboard Controller 
Style)
- IPMI interface which meets the requirement of the BMC 
(Baseboard
- Management Controllers) side for handling the IPMI request 
from

- host system software.
+   tristate

 config ASPEED_KCS_IPMI_BMC
    depends on ARCH_ASPEED || COMPILE_TEST
-   depends on IPMI_KCS_BMC
+   select IPMI_KCS_BMC
    select REGMAP_MMIO
    tristate "Aspeed KCS IPMI BMC driver"
    help

It doesn't make much sense to have IPMI_KCS_BMC on its own.  I was 
going to do this till I saw the compiler error.



Got it, will change it to 'select'
-corey 





--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

Re: [Openipmi-developer] [PATCH arm/aspeed/ast2500 v5 1/2] ipmi: add a KCS IPMI BMC driver

2018-02-02 Thread Wang, Haiyue 

On 2018-02-02 21:52, Corey Minyard wrote:

On 02/01/2018 08:16 PM, Haiyue Wang wrote:

---
v4->v5
- Fix -Wdiscarded-qualifiers 'const' compile warning.
- Fix size_t printk compile error.

v3->v4
- Change to accept WRITE_START any time.

v2->v3

- Update the KCS phase state machine.
- Fix the race condition of read/write.

v1->v2

- Divide the driver into two parts, one handles the BMC KCS IPMI 2.0 
state;
   the other handles the BMC KCS controller such as AST2500 IO 
accessing.
- Use the spin lock APIs to handle the device file operations and BMC 
chip

   IRQ inferface for accessing the same KCS BMC data structure.
- Enhanced the phases handling of the KCS BMC.
- Unified the IOCTL definition for IPMI BMC, it will be used by KCS 
and BT.



Provides a device driver for the KCS (Keyboard Controller Style)
IPMI interface which meets the requirement of the BMC (Baseboard
Management Controllers) side for handling the IPMI request from
host system software.


Ok, this is in my queue, it will go into next once 4.16-rc1 comes out, 
then into

4.16 if all goes well.

Thanks, for your patience and work on this.

*Really appreciate your time on the code review, I really learned more, 
thank you.  :-)


-- Haiyue
*
-corey 



--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer