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 v4 1/2] ipmi: add a KCS IPMI BMC driver

2018-02-01 Thread Corey Minyard 

On 02/01/2018 06:28 PM, Haiyue Wang wrote:

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


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;
 ^
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",
   ^
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.


-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

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

2018-02-01 Thread Haiyue Wang 
---
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.

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..d1751b4
--- /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);
+   read_data(kcs_bmc);
+   write_data(kcs_bmc, KCS_ZERO_DATA);
+
+   kcs_bmc->phase = KCS_PHASE_ERROR;
+   kcs_bmc->data_in_avail = false;
+   kcs_bmc->data_in_idx = 0;
+}
+
+static void kcs_bmc_handle_data(struct kcs_bmc *kcs_bmc)
+{
+   u8 data;
+
+   switch (kcs_bmc->phase) {
+   case KCS_PHASE_WRITE_START:
+