openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] Add Aspeed SSIF BMC driver
@ 2021-03-30 14:10 Quan Nguyen
  2021-03-30 14:10 ` [PATCH v2 1/3] i2c: i2c-core-smbus: Expose PEC calculate function for generic use Quan Nguyen
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Quan Nguyen @ 2021-03-30 14:10 UTC (permalink / raw)
  To: Corey Minyard, Rob Herring, Joel Stanley, Andrew Jeffery,
	Wolfram Sang, Philipp Zabel, openipmi-developer, devicetree,
	linux-arm-kernel, linux-aspeed, linux-kernel, linux-i2c
  Cc: openbmc, Thang Q . Nguyen, Open Source Submission, Phong Vo

This series add support for the Aspeed specific SSIF BMC driver which
is to perform in-band IPMI communication with the host in management
(BMC) side.

v2:
  + Fixed compiling error with COMPILE_TEST for arc

Quan Nguyen (3):
  i2c: i2c-core-smbus: Expose PEC calculate function for generic use
  drivers: char: ipmi: Add Aspeed SSIF BMC driver
  bindings: ipmi: Add binding for Aspeed SSIF BMC driver

 .../bindings/ipmi/aspeed-ssif-bmc.txt         |  18 +
 drivers/char/ipmi/Kconfig                     |  22 +
 drivers/char/ipmi/Makefile                    |   2 +
 drivers/char/ipmi/ssif_bmc.c                  | 645 ++++++++++++++++++
 drivers/char/ipmi/ssif_bmc.h                  |  92 +++
 drivers/char/ipmi/ssif_bmc_aspeed.c           | 132 ++++
 drivers/i2c/i2c-core-smbus.c                  |  12 +-
 include/linux/i2c.h                           |   1 +
 8 files changed, 922 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.txt
 create mode 100644 drivers/char/ipmi/ssif_bmc.c
 create mode 100644 drivers/char/ipmi/ssif_bmc.h
 create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c

-- 
2.28.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/3] i2c: i2c-core-smbus: Expose PEC calculate function for generic use
  2021-03-30 14:10 [PATCH v2 0/3] Add Aspeed SSIF BMC driver Quan Nguyen
@ 2021-03-30 14:10 ` Quan Nguyen
  2021-03-30 14:10 ` [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver Quan Nguyen
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Quan Nguyen @ 2021-03-30 14:10 UTC (permalink / raw)
  To: Corey Minyard, Rob Herring, Joel Stanley, Andrew Jeffery,
	Wolfram Sang, Philipp Zabel, openipmi-developer, devicetree,
	linux-arm-kernel, linux-aspeed, linux-kernel, linux-i2c
  Cc: openbmc, Thang Q . Nguyen, Open Source Submission, Phong Vo

Expose the PEC calculation i2c_smbus_pec() for generic use.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 drivers/i2c/i2c-core-smbus.c | 12 ++++++++++--
 include/linux/i2c.h          |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index d2d32c0fd8c3..e5b2d1465e7e 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -37,8 +37,15 @@ static u8 crc8(u16 data)
 	return (u8)(data >> 8);
 }
 
-/* Incremental CRC8 over count bytes in the array pointed to by p */
-static u8 i2c_smbus_pec(u8 crc, u8 *p, size_t count)
+/**
+ * i2c_smbus_pec - Incremental CRC8 over the given input data array
+ * @crc: previous return crc8 value
+ * @p: pointer to data buffer.
+ * @count: number of bytes in data buffer.
+ *
+ * Incremental CRC8 over count bytes in the array pointed to by p
+ */
+u8 i2c_smbus_pec(u8 crc, u8 *p, size_t count)
 {
 	int i;
 
@@ -46,6 +53,7 @@ static u8 i2c_smbus_pec(u8 crc, u8 *p, size_t count)
 		crc = crc8((crc ^ p[i]) << 8);
 	return crc;
 }
+EXPORT_SYMBOL(i2c_smbus_pec);
 
 /* Assume a 7-bit address, which is reasonable for SMBus */
 static u8 i2c_smbus_msg_pec(u8 pec, struct i2c_msg *msg)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 56622658b215..0d75e5bcdde6 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -144,6 +144,7 @@ s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 /* Now follow the 'nice' access routines. These also document the calling
    conventions of i2c_smbus_xfer. */
 
+u8 i2c_smbus_pec(u8 crc, u8 *p, size_t count);
 s32 i2c_smbus_read_byte(const struct i2c_client *client);
 s32 i2c_smbus_write_byte(const struct i2c_client *client, u8 value);
 s32 i2c_smbus_read_byte_data(const struct i2c_client *client, u8 command);
-- 
2.28.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver
  2021-03-30 14:10 [PATCH v2 0/3] Add Aspeed SSIF BMC driver Quan Nguyen
  2021-03-30 14:10 ` [PATCH v2 1/3] i2c: i2c-core-smbus: Expose PEC calculate function for generic use Quan Nguyen
@ 2021-03-30 14:10 ` Quan Nguyen
  2021-04-02 12:01   ` Philipp Zabel
                     ` (2 more replies)
  2021-03-30 14:10 ` [PATCH v2 3/3] bindings: ipmi: Add binding for " Quan Nguyen
  2021-04-02 14:21 ` [PATCH v2 0/3] Add " Corey Minyard
  3 siblings, 3 replies; 13+ messages in thread
From: Quan Nguyen @ 2021-03-30 14:10 UTC (permalink / raw)
  To: Corey Minyard, Rob Herring, Joel Stanley, Andrew Jeffery,
	Wolfram Sang, Philipp Zabel, openipmi-developer, devicetree,
	linux-arm-kernel, linux-aspeed, linux-kernel, linux-i2c
  Cc: openbmc, Thang Q . Nguyen, Open Source Submission, Phong Vo

The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
in-band IPMI communication with their host in management (BMC) side.

This commits adds support specifically for Aspeed AST2500 which commonly
used as Board Management Controllers.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 drivers/char/ipmi/Kconfig           |  22 +
 drivers/char/ipmi/Makefile          |   2 +
 drivers/char/ipmi/ssif_bmc.c        | 645 ++++++++++++++++++++++++++++
 drivers/char/ipmi/ssif_bmc.h        |  92 ++++
 drivers/char/ipmi/ssif_bmc_aspeed.c | 132 ++++++
 5 files changed, 893 insertions(+)
 create mode 100644 drivers/char/ipmi/ssif_bmc.c
 create mode 100644 drivers/char/ipmi/ssif_bmc.h
 create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 07847d9a459a..45be57023577 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -133,6 +133,28 @@ config ASPEED_BT_IPMI_BMC
 	  found on Aspeed SOCs (AST2400 and AST2500). The driver
 	  implements the BMC side of the BT interface.
 
+config SSIF_IPMI_BMC
+	tristate "SSIF IPMI BMC driver"
+	select I2C
+	select I2C_SLAVE
+	help
+	  This enables the IPMI SMBus system interface (SSIF) at the
+	  management (BMC) side.
+
+	  The driver implements the BMC side of the SMBus system
+	  interface (SSIF).
+
+config ASPEED_SSIF_IPMI_BMC
+	depends on ARCH_ASPEED || COMPILE_TEST
+	select SSIF_IPMI_BMC
+	tristate "Aspeed SSIF IPMI BMC driver"
+	help
+	  Provides a driver for the SSIF IPMI interface found on
+	  Aspeed AST2500 SoC.
+
+	  The driver implements the BMC side of the SMBus system
+	  interface (SSIF), specific for Aspeed AST2500 SoC.
+
 config IPMB_DEVICE_INTERFACE
 	tristate 'IPMB Interface handler'
 	depends on I2C
diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
index 0822adc2ec41..05b993f7335b 100644
--- a/drivers/char/ipmi/Makefile
+++ b/drivers/char/ipmi/Makefile
@@ -27,3 +27,5 @@ obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
 obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
 obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o
 obj-$(CONFIG_IPMB_DEVICE_INTERFACE) += ipmb_dev_int.o
+obj-$(CONFIG_SSIF_IPMI_BMC) += ssif_bmc.o
+obj-$(CONFIG_ASPEED_SSIF_IPMI_BMC) += ssif_bmc_aspeed.o
diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
new file mode 100644
index 000000000000..ae6e8750c795
--- /dev/null
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -0,0 +1,645 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * The driver for BMC side of SSIF interface
+ *
+ * Copyright (c) 2021, Ampere Computing LLC
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+#include <linux/i2c.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+
+#include "ssif_bmc.h"
+
+/*
+ * Call in WRITE context
+ */
+static int send_ssif_bmc_response(struct ssif_bmc_ctx *ssif_bmc, bool non_blocking)
+{
+	unsigned long flags;
+	int ret;
+
+	if (!non_blocking) {
+retry:
+		ret = wait_event_interruptible(ssif_bmc->wait_queue,
+					       !ssif_bmc->response_in_progress);
+		if (ret)
+			return ret;
+	}
+
+	spin_lock_irqsave(&ssif_bmc->lock, flags);
+	if (ssif_bmc->response_in_progress) {
+		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+		if (non_blocking)
+			return -EAGAIN;
+
+		goto retry;
+	}
+
+	/*
+	 * Check the response data length from userspace to determine the type
+	 * of the response message whether it is single-part or multi-part.
+	 */
+	ssif_bmc->is_singlepart_read =
+		(ssif_msg_len(&ssif_bmc->response) <= (MAX_PAYLOAD_PER_TRANSACTION + 1)) ?
+		true : false; /* 1: byte of length */
+
+	ssif_bmc->response_in_progress = true;
+	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+
+	return 0;
+}
+
+/*
+ * Call in READ context
+ */
+static int receive_ssif_bmc_request(struct ssif_bmc_ctx *ssif_bmc, bool non_blocking)
+{
+	unsigned long flags;
+	int ret;
+
+	if (!non_blocking) {
+retry:
+		ret = wait_event_interruptible(ssif_bmc->wait_queue,
+					       ssif_bmc->request_available);
+		if (ret)
+			return ret;
+	}
+
+	spin_lock_irqsave(&ssif_bmc->lock, flags);
+	if (!ssif_bmc->request_available) {
+		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+		if (non_blocking)
+			return -EAGAIN;
+		goto retry;
+	}
+	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+
+	return 0;
+}
+
+/* Handle SSIF message that will be sent to user */
+static ssize_t ssif_bmc_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
+{
+	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
+	struct ssif_msg msg;
+	unsigned long flags;
+	ssize_t ret;
+
+	mutex_lock(&ssif_bmc->file_mutex);
+
+	ret = receive_ssif_bmc_request(ssif_bmc, file->f_flags & O_NONBLOCK);
+	if (ret < 0)
+		goto out;
+
+	spin_lock_irqsave(&ssif_bmc->lock, flags);
+	count = min_t(ssize_t, count, ssif_msg_len(&ssif_bmc->request));
+	memcpy(&msg, &ssif_bmc->request, count);
+	ssif_bmc->request_available = false;
+	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+
+	ret = copy_to_user(buf, &msg, count);
+out:
+	mutex_unlock(&ssif_bmc->file_mutex);
+
+	return (ret < 0) ? ret : count;
+}
+
+/* Handle SSIF message that is written by user */
+static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t count,
+			      loff_t *ppos)
+{
+	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
+	struct ssif_msg msg;
+	unsigned long flags;
+	ssize_t ret;
+
+	if (count > sizeof(struct ssif_msg))
+		return -EINVAL;
+
+	mutex_lock(&ssif_bmc->file_mutex);
+
+	ret = copy_from_user(&msg, buf, count);
+	if (ret)
+		goto out;
+
+	spin_lock_irqsave(&ssif_bmc->lock, flags);
+	if (count >= ssif_msg_len(&ssif_bmc->response))
+		memcpy(&ssif_bmc->response, &msg, count);
+	else
+		ret = -EINVAL;
+	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+
+	if (ret)
+		goto out;
+
+	ret = send_ssif_bmc_response(ssif_bmc, file->f_flags & O_NONBLOCK);
+	if (!ret && ssif_bmc->set_ssif_bmc_status)
+		ssif_bmc->set_ssif_bmc_status(ssif_bmc, SSIF_BMC_READY);
+out:
+	mutex_unlock(&ssif_bmc->file_mutex);
+
+	return (ret < 0) ? ret : count;
+}
+
+static long ssif_bmc_ioctl(struct file *file, unsigned int cmd, unsigned long param)
+{
+	return 0;
+}
+
+static unsigned int ssif_bmc_poll(struct file *file, poll_table *wait)
+{
+	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
+	unsigned int mask = 0;
+
+	mutex_lock(&ssif_bmc->file_mutex);
+	poll_wait(file, &ssif_bmc->wait_queue, wait);
+
+	/*
+	 * The request message is now available so userspace application can
+	 * get the request
+	 */
+	if (ssif_bmc->request_available)
+		mask |= POLLIN;
+
+	mutex_unlock(&ssif_bmc->file_mutex);
+	return mask;
+}
+
+/*
+ * System calls to device interface for user apps
+ */
+static const struct file_operations ssif_bmc_fops = {
+	.owner		= THIS_MODULE,
+	.read		= ssif_bmc_read,
+	.write		= ssif_bmc_write,
+	.poll		= ssif_bmc_poll,
+	.unlocked_ioctl	= ssif_bmc_ioctl,
+};
+
+/* Called with ssif_bmc->lock held. */
+static int handle_request(struct ssif_bmc_ctx *ssif_bmc)
+{
+	if (ssif_bmc->set_ssif_bmc_status)
+		ssif_bmc->set_ssif_bmc_status(ssif_bmc, SSIF_BMC_BUSY);
+
+	/* Request message is available to process */
+	ssif_bmc->request_available = true;
+	/*
+	 * This is the new READ request.
+	 * Clear the response buffer of the previous transaction
+	 */
+	memset(&ssif_bmc->response, 0, sizeof(struct ssif_msg));
+	wake_up_all(&ssif_bmc->wait_queue);
+	return 0;
+}
+
+/* Called with ssif_bmc->lock held. */
+static int complete_response(struct ssif_bmc_ctx *ssif_bmc)
+{
+	/* Invalidate response in buffer to denote it having been sent. */
+	ssif_bmc->response.len = 0;
+	ssif_bmc->response_in_progress = false;
+	ssif_bmc->nbytes_processed = 0;
+	ssif_bmc->remain_len = 0;
+	memset(&ssif_bmc->response_buf, 0, MAX_PAYLOAD_PER_TRANSACTION);
+	wake_up_all(&ssif_bmc->wait_queue);
+	return 0;
+}
+
+static void set_multipart_response_buffer(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
+{
+	u8 response_len = 0;
+	int idx = 0;
+	u8 data_len;
+
+	data_len = ssif_bmc->response.len;
+	switch (ssif_bmc->smbus_cmd) {
+	case SSIF_IPMI_MULTIPART_READ_START:
+		/*
+		 * Read Start length is 32 bytes.
+		 * Read Start transfer first 30 bytes of IPMI response
+		 * and 2 special code 0x00, 0x01.
+		 */
+		*val = MAX_PAYLOAD_PER_TRANSACTION;
+		ssif_bmc->remain_len = data_len - MAX_IPMI_DATA_PER_START_TRANSACTION;
+		ssif_bmc->block_num = 0;
+
+		ssif_bmc->response_buf[idx++] = 0x00; /* Start Flag */
+		ssif_bmc->response_buf[idx++] = 0x01; /* Start Flag */
+		ssif_bmc->response_buf[idx++] = ssif_bmc->response.netfn_lun;
+		ssif_bmc->response_buf[idx++] = ssif_bmc->response.cmd;
+		ssif_bmc->response_buf[idx++] = ssif_bmc->response.payload[0];
+
+		response_len = MAX_PAYLOAD_PER_TRANSACTION - idx;
+
+		memcpy(&ssif_bmc->response_buf[idx], &ssif_bmc->response.payload[1],
+		       response_len);
+		break;
+
+	case SSIF_IPMI_MULTIPART_READ_MIDDLE:
+		/*
+		 * IPMI READ Middle or READ End messages can carry up to 31 bytes
+		 * IPMI data plus block number byte.
+		 */
+		if (ssif_bmc->remain_len < MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION) {
+			/*
+			 * This is READ End message
+			 *  Return length is the remaining response data length
+			 *  plus block number
+			 *  Block number 0xFF is to indicate this is last message
+			 *
+			 * Return length is: remain response plus block number
+			 */
+			*val = ssif_bmc->remain_len + 1;
+			ssif_bmc->block_num = 0xFF;
+			ssif_bmc->response_buf[idx++] = ssif_bmc->block_num;
+			response_len = ssif_bmc->remain_len;
+		} else {
+			/*
+			 * This is READ Middle message
+			 *  Response length is the maximum SMBUS transfer length
+			 *  Block number byte is incremented
+			 * Return length is maximum SMBUS transfer length
+			 */
+			*val = MAX_PAYLOAD_PER_TRANSACTION;
+			ssif_bmc->remain_len -= MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION;
+			response_len = MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION;
+			ssif_bmc->response_buf[idx++] = ssif_bmc->block_num;
+			ssif_bmc->block_num++;
+		}
+
+		memcpy(&ssif_bmc->response_buf[idx],
+		       ssif_bmc->response.payload + 1 + ssif_bmc->nbytes_processed,
+		       response_len);
+		break;
+
+	default:
+		/* Do not expect to go to this case */
+		pr_err("Error: Unexpected SMBus command received 0x%x\n", ssif_bmc->smbus_cmd);
+		break;
+	}
+
+	ssif_bmc->nbytes_processed += response_len;
+}
+
+static void set_singlepart_response_buffer(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
+{
+	u8 *buf = (u8 *)&ssif_bmc->response;
+
+	/*
+	 * Do not expect the IPMI response has data length 0.
+	 * With some I2C SMBus controllers (Aspeed I2C), return 0 for
+	 * the SMBus Read Request callback might cause bad state for
+	 * the bus. So return 1 byte length so that master will
+	 * resend the Read Request because the length of response is
+	 * less than a normal IPMI response.
+	 *
+	 * Otherwise, return the length of IPMI response
+	 */
+	*val = (buf[ssif_bmc->msg_idx]) ? buf[ssif_bmc->msg_idx] : 0x1;
+}
+
+/* Process the IPMI response that will be read by master */
+static void handle_read_processed(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
+{
+	u8 *buf;
+	u8 pec_len, addr, len;
+	u8 pec = 0;
+
+	pec_len = ssif_bmc->pec_support ? 1 : 0;
+	/* PEC - Start Read Address */
+	addr = GET_8BIT_ADDR(ssif_bmc->client->addr);
+	pec = i2c_smbus_pec(pec, &addr, 1);
+	/* PEC - SSIF Command */
+	pec = i2c_smbus_pec(pec, &ssif_bmc->smbus_cmd, 1);
+	/* PEC - Restart Write Address */
+	addr = addr | 0x01;
+	pec = i2c_smbus_pec(pec, &addr, 1);
+
+	if (ssif_bmc->is_singlepart_read) {
+		/* Single-part Read processing */
+		buf = (u8 *)&ssif_bmc->response;
+
+		if (ssif_bmc->response.len && ssif_bmc->msg_idx < ssif_bmc->response.len) {
+			ssif_bmc->msg_idx++;
+			*val = buf[ssif_bmc->msg_idx];
+		} else if (ssif_bmc->response.len &&
+			   (ssif_bmc->msg_idx == ssif_bmc->response.len)) {
+			ssif_bmc->msg_idx++;
+			*val = i2c_smbus_pec(pec, buf, ssif_msg_len(&ssif_bmc->response));
+		} else {
+			*val = 0;
+		}
+		/* Invalidate response buffer to denote it is sent */
+		if (ssif_bmc->msg_idx + 1 >= (ssif_msg_len(&ssif_bmc->response) + pec_len))
+			complete_response(ssif_bmc);
+	} else {
+		/* Multi-part Read processing */
+		switch (ssif_bmc->smbus_cmd) {
+		case SSIF_IPMI_MULTIPART_READ_START:
+		case SSIF_IPMI_MULTIPART_READ_MIDDLE:
+			buf = (u8 *)&ssif_bmc->response_buf;
+			*val = buf[ssif_bmc->msg_idx];
+			ssif_bmc->msg_idx++;
+			break;
+		default:
+			/* Do not expect to go to this case */
+			pr_err("Error: Unexpected SMBus command received 0x%x\n",
+			       ssif_bmc->smbus_cmd);
+			break;
+		}
+		len = (ssif_bmc->block_num == 0xFF) ?
+		       ssif_bmc->remain_len + 1 : MAX_PAYLOAD_PER_TRANSACTION;
+		if (ssif_bmc->msg_idx == (len + 1)) {
+			pec = i2c_smbus_pec(pec, &len, 1);
+			*val = i2c_smbus_pec(pec, ssif_bmc->response_buf, len);
+		}
+		/* Invalidate response buffer to denote last response is sent */
+		if (ssif_bmc->block_num == 0xFF &&
+		    ssif_bmc->msg_idx > (ssif_bmc->remain_len + pec_len)) {
+			complete_response(ssif_bmc);
+		}
+	}
+}
+
+static void handle_write_received(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
+{
+	u8 *buf;
+	u8 smbus_cmd;
+
+	buf = (u8 *)&ssif_bmc->request;
+	if (ssif_bmc->msg_idx >= sizeof(struct ssif_msg))
+		return;
+
+	smbus_cmd = ssif_bmc->smbus_cmd;
+	switch (smbus_cmd) {
+	case SSIF_IPMI_SINGLEPART_WRITE:
+		/* Single-part write */
+		buf[ssif_bmc->msg_idx - 1] = *val;
+		ssif_bmc->msg_idx++;
+
+		break;
+	case SSIF_IPMI_MULTIPART_WRITE_START:
+		/* Reset length to zero */
+		if (ssif_bmc->msg_idx == 1)
+			ssif_bmc->request.len = 0;
+
+		fallthrough;
+	case SSIF_IPMI_MULTIPART_WRITE_MIDDLE:
+	case SSIF_IPMI_MULTIPART_WRITE_END:
+		/* Multi-part write, 2nd byte received is length */
+		if (ssif_bmc->msg_idx == 1) {
+			ssif_bmc->request.len += *val;
+			ssif_bmc->recv_len = *val;
+		} else {
+			buf[ssif_bmc->msg_idx - 1 +
+			    ssif_bmc->request.len - ssif_bmc->recv_len]	= *val;
+		}
+
+		ssif_bmc->msg_idx++;
+
+		break;
+	default:
+		/* Do not expect to go to this case */
+		pr_err("Error: Unexpected SMBus command received 0x%x\n", ssif_bmc->smbus_cmd);
+		break;
+	}
+}
+
+static bool validate_pec(struct ssif_bmc_ctx *ssif_bmc)
+{
+	u8 rpec = 0, cpec = 0;
+	bool ret = true;
+	u8 addr, index;
+	u8 *buf;
+
+	buf = (u8 *)&ssif_bmc->request;
+	switch (ssif_bmc->smbus_cmd) {
+	case SSIF_IPMI_SINGLEPART_WRITE:
+		if ((ssif_bmc->msg_idx - 1) == ssif_msg_len(&ssif_bmc->request)) {
+			/* PEC is not included */
+			ssif_bmc->pec_support = false;
+			return true;
+		}
+
+		if ((ssif_bmc->msg_idx - 1) != (ssif_msg_len(&ssif_bmc->request) + 1))
+			goto error;
+
+		/* PEC is included */
+		ssif_bmc->pec_support = true;
+		rpec = buf[ssif_bmc->msg_idx - 2];
+		addr = GET_8BIT_ADDR(ssif_bmc->client->addr);
+		cpec = i2c_smbus_pec(cpec, &addr, 1);
+		cpec = i2c_smbus_pec(cpec, &ssif_bmc->smbus_cmd, 1);
+		cpec = i2c_smbus_pec(cpec, buf, ssif_msg_len(&ssif_bmc->request));
+		if (rpec != cpec) {
+			pr_err("Bad PEC 0x%02x vs. 0x%02x\n", rpec, cpec);
+			ret = false;
+		}
+
+		break;
+	case SSIF_IPMI_MULTIPART_WRITE_START:
+	case SSIF_IPMI_MULTIPART_WRITE_MIDDLE:
+	case SSIF_IPMI_MULTIPART_WRITE_END:
+		index = ssif_bmc->request.len - ssif_bmc->recv_len;
+		if ((ssif_bmc->msg_idx - 1 + index) == ssif_msg_len(&ssif_bmc->request)) {
+			/* PEC is not included */
+			ssif_bmc->pec_support = false;
+			return true;
+		}
+
+		if ((ssif_bmc->msg_idx - 1 + index) != (ssif_msg_len(&ssif_bmc->request) + 1))
+			goto error;
+
+		/* PEC is included */
+		ssif_bmc->pec_support = true;
+		rpec = buf[ssif_bmc->msg_idx - 2 + index];
+		addr = GET_8BIT_ADDR(ssif_bmc->client->addr);
+		cpec = i2c_smbus_pec(cpec, &addr, 1);
+		cpec = i2c_smbus_pec(cpec, &ssif_bmc->smbus_cmd, 1);
+		cpec = i2c_smbus_pec(cpec, &ssif_bmc->recv_len, 1);
+		/* As SMBus specification does not allow the length
+		 * (byte count) in the Write-Block protocol to be zero.
+		 * Therefore, it is illegal to have the last Middle
+		 * transaction in the sequence carry 32-bytes and have
+		 * a length of ‘0’ in the End transaction.
+		 * But some users may try to use this way and we should
+		 * prevent ssif_bmc driver broken in this case.
+		 */
+		if (ssif_bmc->recv_len != 0)
+			cpec = i2c_smbus_pec(cpec, buf + 1 + index, ssif_bmc->recv_len);
+
+		if (rpec != cpec) {
+			pr_err("Bad PEC 0x%02x vs. 0x%02x\n", rpec, cpec);
+			ret = false;
+		}
+
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+error:
+	/* Do not expect to go to this case */
+	pr_err("Error: Unexpected length received %d\n", ssif_msg_len(&ssif_bmc->request));
+
+	return false;
+}
+
+static void complete_write_received(struct ssif_bmc_ctx *ssif_bmc)
+{
+	u8 cmd = ssif_bmc->smbus_cmd;
+
+	/* A BMC that receives an invalid PEC shall drop the data for the write
+	 * transaction and any further transactions (read or write) until
+	 * the next valid read or write Start transaction is received
+	 */
+	if (!validate_pec(ssif_bmc)) {
+		pr_err("Received invalid PEC\n");
+		return;
+	}
+
+	if (cmd == SSIF_IPMI_SINGLEPART_WRITE || cmd == SSIF_IPMI_MULTIPART_WRITE_END)
+		handle_request(ssif_bmc);
+}
+
+/*
+ * Callback function to handle I2C slave events
+ */
+static int ssif_bmc_cb(struct i2c_client *client, enum i2c_slave_event event, u8 *val)
+{
+	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
+
+	spin_lock(&ssif_bmc->lock);
+
+	/* I2C Event Handler:
+	 *   I2C_SLAVE_READ_REQUESTED	0x0
+	 *   I2C_SLAVE_WRITE_REQUESTED	0x1
+	 *   I2C_SLAVE_READ_PROCESSED	0x2
+	 *   I2C_SLAVE_WRITE_RECEIVED	0x3
+	 *   I2C_SLAVE_STOP		0x4
+	 */
+	switch (event) {
+	case I2C_SLAVE_READ_REQUESTED:
+		ssif_bmc->msg_idx = 0;
+		if (ssif_bmc->is_singlepart_read)
+			set_singlepart_response_buffer(ssif_bmc, val);
+		else
+			set_multipart_response_buffer(ssif_bmc, val);
+		break;
+
+	case I2C_SLAVE_WRITE_REQUESTED:
+		ssif_bmc->msg_idx = 0;
+		break;
+
+	case I2C_SLAVE_READ_PROCESSED:
+		handle_read_processed(ssif_bmc, val);
+		break;
+
+	case I2C_SLAVE_WRITE_RECEIVED:
+		/*
+		 * First byte is SMBUS command, not a part of SSIF message.
+		 * SSIF request buffer starts with msg_idx 1 for the first
+		 *  buffer byte.
+		 */
+		if (ssif_bmc->msg_idx == 0) {
+			/* SMBUS command can vary (single or multi-part) */
+			ssif_bmc->smbus_cmd = *val;
+			ssif_bmc->msg_idx++;
+		} else {
+			handle_write_received(ssif_bmc, val);
+		}
+
+		break;
+
+	case I2C_SLAVE_STOP:
+		/*
+		 * PEC byte is appended at the end of each transaction.
+		 * Detect PEC is support or not after receiving write request
+		 * completely.
+		 */
+		if (ssif_bmc->last_event == I2C_SLAVE_WRITE_RECEIVED)
+			complete_write_received(ssif_bmc);
+		/* Reset message index */
+		ssif_bmc->msg_idx = 0;
+		break;
+
+	default:
+		break;
+	}
+	ssif_bmc->last_event = event;
+	spin_unlock(&ssif_bmc->lock);
+
+	return 0;
+}
+
+struct ssif_bmc_ctx *ssif_bmc_alloc(struct i2c_client *client, int sizeof_priv)
+{
+	struct ssif_bmc_ctx *ssif_bmc;
+	int ret;
+
+	ssif_bmc = devm_kzalloc(&client->dev, sizeof(*ssif_bmc) + sizeof_priv, GFP_KERNEL);
+	if (!ssif_bmc)
+		return ERR_PTR(-ENOMEM);
+
+	spin_lock_init(&ssif_bmc->lock);
+
+	init_waitqueue_head(&ssif_bmc->wait_queue);
+	ssif_bmc->request_available = false;
+	ssif_bmc->response_in_progress = false;
+
+	mutex_init(&ssif_bmc->file_mutex);
+
+	/* Register misc device interface */
+	ssif_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
+	ssif_bmc->miscdev.name = DEVICE_NAME;
+	ssif_bmc->miscdev.fops = &ssif_bmc_fops;
+	ssif_bmc->miscdev.parent = &client->dev;
+	ret = misc_register(&ssif_bmc->miscdev);
+	if (ret)
+		goto out;
+
+	ssif_bmc->client = client;
+	ssif_bmc->client->flags |= I2C_CLIENT_SLAVE;
+
+	/* Register I2C slave */
+	i2c_set_clientdata(client, ssif_bmc);
+	ret = i2c_slave_register(client, ssif_bmc_cb);
+	if (ret) {
+		misc_deregister(&ssif_bmc->miscdev);
+		goto out;
+	}
+
+	return ssif_bmc;
+
+out:
+	devm_kfree(&client->dev, ssif_bmc);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(ssif_bmc_alloc);
+
+MODULE_AUTHOR("Chuong Tran <chuong@os.amperecomputing.com>");
+MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
+MODULE_DESCRIPTION("Linux device driver of the BMC IPMI SSIF interface.");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/char/ipmi/ssif_bmc.h b/drivers/char/ipmi/ssif_bmc.h
new file mode 100644
index 000000000000..a2ee090572db
--- /dev/null
+++ b/drivers/char/ipmi/ssif_bmc.h
@@ -0,0 +1,92 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * The driver for BMC side of SSIF interface
+ *
+ * Copyright (c) 2021, Ampere Computing LLC
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+#ifndef __SSIF_BMC_H__
+#define __SSIF_BMC_H__
+
+#define DEVICE_NAME				"ipmi-ssif-host"
+
+#define GET_8BIT_ADDR(addr_7bit)		(((addr_7bit) << 1) & 0xff)
+
+#define MSG_PAYLOAD_LEN_MAX			252
+
+/* A standard SMBus Transaction is limited to 32 data bytes */
+#define MAX_PAYLOAD_PER_TRANSACTION		32
+
+#define MAX_IPMI_DATA_PER_START_TRANSACTION	30
+#define MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION	31
+
+#define SSIF_IPMI_SINGLEPART_WRITE		0x2
+#define SSIF_IPMI_SINGLEPART_READ		0x3
+#define SSIF_IPMI_MULTIPART_WRITE_START		0x6
+#define SSIF_IPMI_MULTIPART_WRITE_MIDDLE	0x7
+#define SSIF_IPMI_MULTIPART_WRITE_END		0x8
+#define SSIF_IPMI_MULTIPART_READ_START		0x3
+#define SSIF_IPMI_MULTIPART_READ_MIDDLE		0x9
+
+struct ssif_msg {
+	u8 len;
+	u8 netfn_lun;
+	u8 cmd;
+	u8 payload[MSG_PAYLOAD_LEN_MAX];
+} __packed;
+
+static inline u32 ssif_msg_len(struct ssif_msg *ssif_msg)
+{
+	return ssif_msg->len + 1;
+}
+
+#define SSIF_BMC_BUSY   0x01
+#define SSIF_BMC_READY  0x02
+
+struct ssif_bmc_ctx {
+	struct i2c_client	*client;
+	struct miscdevice	miscdev;
+	u8			smbus_cmd;
+	struct ssif_msg		request;
+	bool			request_available;
+	struct ssif_msg		response;
+	bool			response_in_progress;
+	/* Response buffer for Multi-part Read Transaction */
+	u8			response_buf[MAX_PAYLOAD_PER_TRANSACTION];
+	/* Flag to identify a Multi-part Read Transaction */
+	bool			is_singlepart_read;
+	u8			nbytes_processed;
+	u8			remain_len;
+	u8			recv_len;
+	/* Block Number of a Multi-part Read Transaction */
+	u8			block_num;
+	size_t			msg_idx;
+	enum i2c_slave_event	last_event;
+	bool			pec_support;
+	spinlock_t		lock;
+	wait_queue_head_t	wait_queue;
+	struct mutex		file_mutex;
+	void (*set_ssif_bmc_status)(struct ssif_bmc_ctx *ssif_bmc, unsigned int flags);
+	void			*priv;
+};
+
+static inline struct ssif_bmc_ctx *to_ssif_bmc(struct file *file)
+{
+	return container_of(file->private_data, struct ssif_bmc_ctx, miscdev);
+}
+
+struct ssif_bmc_ctx *ssif_bmc_alloc(struct i2c_client *client, int sizeof_priv);
+
+#endif /* __SSIF_BMC_H__ */
diff --git a/drivers/char/ipmi/ssif_bmc_aspeed.c b/drivers/char/ipmi/ssif_bmc_aspeed.c
new file mode 100644
index 000000000000..a563fcff5acc
--- /dev/null
+++ b/drivers/char/ipmi/ssif_bmc_aspeed.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * The driver for BMC side of Aspeed SSIF interface
+ *
+ * Copyright (c) 2021, Ampere Computing LLC
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+#include <linux/i2c.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/poll.h>
+#include <linux/iopoll.h>
+
+#include "ssif_bmc.h"
+
+struct aspeed_i2c_bus {
+	struct i2c_adapter              adap;
+	struct device                   *dev;
+	void __iomem                    *base;
+	struct reset_control            *rst;
+	/* Synchronizes I/O mem access to base. */
+	spinlock_t                      lock;
+};
+
+#define ASPEED_I2C_INTR_CTRL_REG	0x0c
+#define ASPEED_I2CD_INTR_SLAVE_MATCH	BIT(7)
+#define ASPEED_I2CD_INTR_RX_DONE	BIT(2)
+static void aspeed_i2c_enable_interrupt(struct aspeed_i2c_bus *bus, unsigned long mask)
+{
+	unsigned long current_mask;
+
+	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
+	writel(current_mask | mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
+}
+
+static void aspeed_i2c_disable_interrupt(struct aspeed_i2c_bus *bus, unsigned long mask)
+{
+	unsigned long current_mask;
+
+	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
+	writel(current_mask & ~mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
+}
+
+static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc, unsigned int status)
+{
+	struct aspeed_i2c_bus *bus;
+	unsigned long flags;
+
+	bus = (struct aspeed_i2c_bus *)ssif_bmc->priv;
+	if (!bus)
+		return;
+
+	spin_lock_irqsave(&bus->lock, flags);
+
+	if (status & SSIF_BMC_BUSY) {
+		/* Ignore RX_DONE and SLAVE_MATCH when slave busy processing */
+		aspeed_i2c_disable_interrupt(bus, ASPEED_I2CD_INTR_RX_DONE);
+		aspeed_i2c_disable_interrupt(bus, ASPEED_I2CD_INTR_SLAVE_MATCH);
+	} else if (status & SSIF_BMC_READY) {
+		/* Enable RX_DONE and SLAVE_MATCH when slave ready */
+		aspeed_i2c_enable_interrupt(bus, ASPEED_I2CD_INTR_RX_DONE);
+		aspeed_i2c_enable_interrupt(bus, ASPEED_I2CD_INTR_SLAVE_MATCH);
+	}
+
+	spin_unlock_irqrestore(&bus->lock, flags);
+}
+
+static int ssif_bmc_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct ssif_bmc_ctx *ssif_bmc;
+
+	ssif_bmc = ssif_bmc_alloc(client, sizeof(struct aspeed_i2c_bus));
+	if (IS_ERR(ssif_bmc))
+		return PTR_ERR(ssif_bmc);
+
+	ssif_bmc->priv = i2c_get_adapdata(client->adapter);
+	ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status;
+
+	return 0;
+}
+
+static int ssif_bmc_remove(struct i2c_client *client)
+{
+	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
+
+	i2c_slave_unregister(client);
+	misc_deregister(&ssif_bmc->miscdev);
+
+	return 0;
+}
+
+static const struct of_device_id ssif_bmc_match[] = {
+	{ .compatible = "aspeed,ast2500-ssif-bmc" },
+	{ },
+};
+
+static const struct i2c_device_id ssif_bmc_id[] = {
+	{ DEVICE_NAME, 0 },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(i2c, ssif_bmc_id);
+
+static struct i2c_driver ssif_bmc_driver = {
+	.driver		= {
+		.name		= DEVICE_NAME,
+		.of_match_table = ssif_bmc_match,
+	},
+	.probe		= ssif_bmc_probe,
+	.remove		= ssif_bmc_remove,
+	.id_table	= ssif_bmc_id,
+};
+
+module_i2c_driver(ssif_bmc_driver);
+
+MODULE_AUTHOR("Chuong Tran <chuong@os.amperecomputing.com>");
+MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
+MODULE_DESCRIPTION("Linux device driver of Aspeed BMC IPMI SSIF interface.");
+MODULE_LICENSE("GPL v2");
-- 
2.28.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 3/3] bindings: ipmi: Add binding for Aspeed SSIF BMC driver
  2021-03-30 14:10 [PATCH v2 0/3] Add Aspeed SSIF BMC driver Quan Nguyen
  2021-03-30 14:10 ` [PATCH v2 1/3] i2c: i2c-core-smbus: Expose PEC calculate function for generic use Quan Nguyen
  2021-03-30 14:10 ` [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver Quan Nguyen
@ 2021-03-30 14:10 ` Quan Nguyen
  2021-04-01 17:09   ` Rob Herring
  2021-04-02 14:21 ` [PATCH v2 0/3] Add " Corey Minyard
  3 siblings, 1 reply; 13+ messages in thread
From: Quan Nguyen @ 2021-03-30 14:10 UTC (permalink / raw)
  To: Corey Minyard, Rob Herring, Joel Stanley, Andrew Jeffery,
	Wolfram Sang, Philipp Zabel, openipmi-developer, devicetree,
	linux-arm-kernel, linux-aspeed, linux-kernel, linux-i2c
  Cc: openbmc, Thang Q . Nguyen, Open Source Submission, Phong Vo

Add device tree binding document for the Aspeed SSIF BMC driver.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 .../bindings/ipmi/aspeed-ssif-bmc.txt          | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.txt

diff --git a/Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.txt b/Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.txt
new file mode 100644
index 000000000000..1616f0188db9
--- /dev/null
+++ b/Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.txt
@@ -0,0 +1,18 @@
+# Aspeed SSIF (SMBus system interface) IPMI BMC interface
+
+The Aspeed AST2500 are commonly used as BMCs (Baseboard Management Controllers)
+and the SSIF slave interface can be used to perform in-band IPMI communication
+with their host.
+
+Required properties:
+
+- compatible : should be
+       "aspeed,ast2500-ssif-bmc"
+- reg: I2C address the registers
+
+Example:
+
+       ssif-bmc@10 {
+               compatible = "aspeed,ast2500-ssif-bmc";
+               reg = <0x10>;
+       };
-- 
2.28.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/3] bindings: ipmi: Add binding for Aspeed SSIF BMC driver
  2021-03-30 14:10 ` [PATCH v2 3/3] bindings: ipmi: Add binding for " Quan Nguyen
@ 2021-04-01 17:09   ` Rob Herring
  2021-04-02  1:59     ` Quan Nguyen
  0 siblings, 1 reply; 13+ messages in thread
From: Rob Herring @ 2021-04-01 17:09 UTC (permalink / raw)
  To: Quan Nguyen
  Cc: devicetree, linux-aspeed, Corey Minyard, Andrew Jeffery, openbmc,
	Thang Q . Nguyen, linux-kernel, Phong Vo, Wolfram Sang,
	Philipp Zabel, openipmi-developer, Open Source Submission,
	linux-arm-kernel, linux-i2c

On Tue, Mar 30, 2021 at 09:10:29PM +0700, Quan Nguyen wrote:
> Add device tree binding document for the Aspeed SSIF BMC driver.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
>  .../bindings/ipmi/aspeed-ssif-bmc.txt          | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.txt

Same comment as you ignored on v1.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/3] bindings: ipmi: Add binding for Aspeed SSIF BMC driver
  2021-04-01 17:09   ` Rob Herring
@ 2021-04-02  1:59     ` Quan Nguyen
  0 siblings, 0 replies; 13+ messages in thread
From: Quan Nguyen @ 2021-04-02  1:59 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, linux-aspeed, Corey Minyard, Andrew Jeffery, openbmc,
	Thang Q . Nguyen, linux-kernel, Phong Vo, Wolfram Sang,
	Philipp Zabel, openipmi-developer, Open Source Submission,
	linux-arm-kernel, linux-i2c

On 02/04/2021 00:09, Rob Herring wrote:
> On Tue, Mar 30, 2021 at 09:10:29PM +0700, Quan Nguyen wrote:
>> Add device tree binding document for the Aspeed SSIF BMC driver.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>>   .../bindings/ipmi/aspeed-ssif-bmc.txt          | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.txt
> 
> Same comment as you ignored on v1.
> 

Dear Rob,
I really did not mean to do that.

What happen is that there was a compilation error complaint by kernel 
robot test on my v1. So I tried to send my v2 to fix that issue asap. 
Unfortunately, your reply on my v1 arrived just right after I hit "git 
send-email" to send out my v2.

For this comment, I'll switch to use yaml file in next version.

- Quan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver
  2021-03-30 14:10 ` [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver Quan Nguyen
@ 2021-04-02 12:01   ` Philipp Zabel
  2021-04-07  7:23     ` Quan Nguyen
  2021-04-07 15:50   ` Corey Minyard
  2021-04-22 10:49   ` Graeme Gregory
  2 siblings, 1 reply; 13+ messages in thread
From: Philipp Zabel @ 2021-04-02 12:01 UTC (permalink / raw)
  To: Quan Nguyen
  Cc: devicetree, linux-aspeed, Corey Minyard, Andrew Jeffery, openbmc,
	Thang Q . Nguyen, linux-kernel, Phong Vo, Wolfram Sang,
	Rob Herring, openipmi-developer, Open Source Submission,
	linux-arm-kernel, linux-i2c

Hi Quan,

On Tue, Mar 30, 2021 at 09:10:28PM +0700, Quan Nguyen wrote:
> The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
> in-band IPMI communication with their host in management (BMC) side.
> 
> This commits adds support specifically for Aspeed AST2500 which commonly
> used as Board Management Controllers.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
[...]
> diff --git a/drivers/char/ipmi/ssif_bmc_aspeed.c b/drivers/char/ipmi/ssif_bmc_aspeed.c
> new file mode 100644
> index 000000000000..a563fcff5acc
> --- /dev/null
> +++ b/drivers/char/ipmi/ssif_bmc_aspeed.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * The driver for BMC side of Aspeed SSIF interface
> + *
> + * Copyright (c) 2021, Ampere Computing LLC
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <https://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/poll.h>
> +#include <linux/iopoll.h>
> +
> +#include "ssif_bmc.h"
> +
> +struct aspeed_i2c_bus {
> +	struct i2c_adapter              adap;
> +	struct device                   *dev;

This device handle is apparently unused.

> +	void __iomem                    *base;
> +	struct reset_control            *rst;

This reset control handle is unused as well.

> +	/* Synchronizes I/O mem access to base. */
> +	spinlock_t                      lock;
> +};

regards
Philipp

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/3] Add Aspeed SSIF BMC driver
  2021-03-30 14:10 [PATCH v2 0/3] Add Aspeed SSIF BMC driver Quan Nguyen
                   ` (2 preceding siblings ...)
  2021-03-30 14:10 ` [PATCH v2 3/3] bindings: ipmi: Add binding for " Quan Nguyen
@ 2021-04-02 14:21 ` Corey Minyard
  2021-04-07 13:09   ` Quan Nguyen
  3 siblings, 1 reply; 13+ messages in thread
From: Corey Minyard @ 2021-04-02 14:21 UTC (permalink / raw)
  To: Quan Nguyen
  Cc: devicetree, linux-aspeed, Andrew Jeffery, openbmc,
	Thang Q . Nguyen, linux-kernel, Phong Vo, Wolfram Sang,
	Rob Herring, Philipp Zabel, openipmi-developer,
	Open Source Submission, linux-arm-kernel, linux-i2c

On Tue, Mar 30, 2021 at 09:10:26PM +0700, Quan Nguyen wrote:
> This series add support for the Aspeed specific SSIF BMC driver which
> is to perform in-band IPMI communication with the host in management
> (BMC) side.

I don't have any specific feedback for this, but I'm wondering if it's
really necessary.

Why can't the BMC just open the I2C device and use it?  Is there any
functionality that this provides that cannot be accomplished from
userland access to the I2C device?  I don't see any.

If it tied into some existing framework to give abstract access to a BMC
slave side interface, I'd be ok with this.  But I don't see that.

Unless there is a big need to have this in the kernel, I'm against
including this and would suggest you do all this work in userland.
Perhaps write a library.  Sorry, but I'm trying to do my part to reduce
unnecessary things in the kernel.

Thanks,

-corey

> 
> v2:
>   + Fixed compiling error with COMPILE_TEST for arc
> 
> Quan Nguyen (3):
>   i2c: i2c-core-smbus: Expose PEC calculate function for generic use
>   drivers: char: ipmi: Add Aspeed SSIF BMC driver
>   bindings: ipmi: Add binding for Aspeed SSIF BMC driver
> 
>  .../bindings/ipmi/aspeed-ssif-bmc.txt         |  18 +
>  drivers/char/ipmi/Kconfig                     |  22 +
>  drivers/char/ipmi/Makefile                    |   2 +
>  drivers/char/ipmi/ssif_bmc.c                  | 645 ++++++++++++++++++
>  drivers/char/ipmi/ssif_bmc.h                  |  92 +++
>  drivers/char/ipmi/ssif_bmc_aspeed.c           | 132 ++++
>  drivers/i2c/i2c-core-smbus.c                  |  12 +-
>  include/linux/i2c.h                           |   1 +
>  8 files changed, 922 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.txt
>  create mode 100644 drivers/char/ipmi/ssif_bmc.c
>  create mode 100644 drivers/char/ipmi/ssif_bmc.h
>  create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c
> 
> -- 
> 2.28.0
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver
  2021-04-02 12:01   ` Philipp Zabel
@ 2021-04-07  7:23     ` Quan Nguyen
  0 siblings, 0 replies; 13+ messages in thread
From: Quan Nguyen @ 2021-04-07  7:23 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: devicetree, linux-aspeed, Corey Minyard, Andrew Jeffery, openbmc,
	Thang Q . Nguyen, linux-kernel, Phong Vo, Wolfram Sang,
	Rob Herring, openipmi-developer, Open Source Submission,
	linux-arm-kernel, linux-i2c

On 02/04/2021 19:01, Philipp Zabel wrote:
> Hi Quan,
> 
> On Tue, Mar 30, 2021 at 09:10:28PM +0700, Quan Nguyen wrote:
>> The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
>> in-band IPMI communication with their host in management (BMC) side.
>>
>> This commits adds support specifically for Aspeed AST2500 which commonly
>> used as Board Management Controllers.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
> [...]
>> diff --git a/drivers/char/ipmi/ssif_bmc_aspeed.c b/drivers/char/ipmi/ssif_bmc_aspeed.c
>> new file mode 100644
>> index 000000000000..a563fcff5acc
>> --- /dev/null
>> +++ b/drivers/char/ipmi/ssif_bmc_aspeed.c
>> @@ -0,0 +1,132 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * The driver for BMC side of Aspeed SSIF interface
>> + *
>> + * Copyright (c) 2021, Ampere Computing LLC
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <https://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/module.h>
>> +#include <linux/poll.h>
>> +#include <linux/iopoll.h>
>> +
>> +#include "ssif_bmc.h"
>> +
>> +struct aspeed_i2c_bus {
>> +	struct i2c_adapter              adap;
>> +	struct device                   *dev;
> 
> This device handle is apparently unused.
> 
>> +	void __iomem                    *base;
>> +	struct reset_control            *rst;
> 
> This reset control handle is unused as well.
> 

Thank for the comment, Philipp.

The main purpose here is to get the aspeed_i2c_bus->base of aspeed_i2c 
driver so that the ASPEED_I2CD_INTR_RX_DONE and 
ASPEED_I2CD_INTR_SLAVE_MATCH can be masked while handling the incoming 
request, otherwise, the process is disturbed because these interrupts 
keep occurring with unwanted value.

Other solution we have in mind is to implement and expose the 
EXPORT_SYMBOL_GPL(i2c_aspeed_configure_slave) from 
drivers/i2c/busses/i2c-aspeed.c

Really appreciate if you could comment more on the solution.

Thanks,
- Quan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/3] Add Aspeed SSIF BMC driver
  2021-04-02 14:21 ` [PATCH v2 0/3] Add " Corey Minyard
@ 2021-04-07 13:09   ` Quan Nguyen
  2021-04-07 14:28     ` Corey Minyard
  0 siblings, 1 reply; 13+ messages in thread
From: Quan Nguyen @ 2021-04-07 13:09 UTC (permalink / raw)
  To: minyard
  Cc: devicetree, linux-aspeed, Andrew Jeffery, openbmc,
	Thang Q . Nguyen, linux-kernel, Phong Vo, Wolfram Sang,
	Rob Herring, Philipp Zabel, openipmi-developer,
	Open Source Submission, linux-arm-kernel, linux-i2c

Hi Corey,

Thank you for reviewing
I'll put my respond inline below.

-Quan

On 02/04/2021 21:21, Corey Minyard wrote:
> On Tue, Mar 30, 2021 at 09:10:26PM +0700, Quan Nguyen wrote:
>> This series add support for the Aspeed specific SSIF BMC driver which
>> is to perform in-band IPMI communication with the host in management
>> (BMC) side.
> 
> I don't have any specific feedback for this, but I'm wondering if it's
> really necessary.
> 
> Why can't the BMC just open the I2C device and use it?  Is there any
> functionality that this provides that cannot be accomplished from
> userland access to the I2C device?  I don't see any.
>
> If it tied into some existing framework to give abstract access to a BMC
> slave side interface, I'd be ok with this.  But I don't see that.
> 

The SSIF at the BMC side acts as an I2C slave and we think that the 
kernel driver is unavoidable to handle the I2c slave events 
(https://www.kernel.org/doc/html/latest/i2c/slave-interface.html)

And to make it works with existing OpenBMC IPMI stack, a userspace part, 
ssifbridge, is needed (https://github.com/openbmc/ssifbridge). This 
ssifbridge is to connect this driver with the OpenBMC IPMI stack so the 
IPMI stack can communicate via SSIF channel in similar way that was 
implemented with BT and KCS (ie: btbridge/kcsbridge and its corespondent 
kernel drivers (https://github.com/openbmc/btbridge and 
https://github.com/openbmc/kcsbridge))

> Unless there is a big need to have this in the kernel, I'm against
> including this and would suggest you do all this work in userland.
> Perhaps write a library.  Sorry, but I'm trying to do my part to reduce
> unnecessary things in the kernel.
> 
> Thanks,
> 
> -corey
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/3] Add Aspeed SSIF BMC driver
  2021-04-07 13:09   ` Quan Nguyen
@ 2021-04-07 14:28     ` Corey Minyard
  0 siblings, 0 replies; 13+ messages in thread
From: Corey Minyard @ 2021-04-07 14:28 UTC (permalink / raw)
  To: Quan Nguyen
  Cc: devicetree, linux-aspeed, Andrew Jeffery, openbmc,
	Thang Q . Nguyen, linux-kernel, Phong Vo, Wolfram Sang,
	Rob Herring, Philipp Zabel, openipmi-developer,
	Open Source Submission, linux-arm-kernel, linux-i2c

On Wed, Apr 07, 2021 at 08:09:50PM +0700, Quan Nguyen wrote:
> Hi Corey,
> 
> Thank you for reviewing
> I'll put my respond inline below.
> 
> -Quan
> 
> On 02/04/2021 21:21, Corey Minyard wrote:
> > On Tue, Mar 30, 2021 at 09:10:26PM +0700, Quan Nguyen wrote:
> > > This series add support for the Aspeed specific SSIF BMC driver which
> > > is to perform in-band IPMI communication with the host in management
> > > (BMC) side.
> > 
> > I don't have any specific feedback for this, but I'm wondering if it's
> > really necessary.
> > 
> > Why can't the BMC just open the I2C device and use it?  Is there any
> > functionality that this provides that cannot be accomplished from
> > userland access to the I2C device?  I don't see any.
> > 
> > If it tied into some existing framework to give abstract access to a BMC
> > slave side interface, I'd be ok with this.  But I don't see that.
> > 
> 
> The SSIF at the BMC side acts as an I2C slave and we think that the kernel
> driver is unavoidable to handle the I2c slave events
> (https://www.kernel.org/doc/html/latest/i2c/slave-interface.html)
> 
> And to make it works with existing OpenBMC IPMI stack, a userspace part,
> ssifbridge, is needed (https://github.com/openbmc/ssifbridge). This
> ssifbridge is to connect this driver with the OpenBMC IPMI stack so the IPMI
> stack can communicate via SSIF channel in similar way that was implemented
> with BT and KCS (ie: btbridge/kcsbridge and its corespondent kernel drivers
> (https://github.com/openbmc/btbridge and
> https://github.com/openbmc/kcsbridge))

Dang, I don't know why there's not a generic userland interface for
the slave.  And I've made this mistake before :(.

Anyway, you are right, you need a driver.  I'll review.

-corey

> 
> > Unless there is a big need to have this in the kernel, I'm against
> > including this and would suggest you do all this work in userland.
> > Perhaps write a library.  Sorry, but I'm trying to do my part to reduce
> > unnecessary things in the kernel.
> > 
> > Thanks,
> > 
> > -corey
> > 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver
  2021-03-30 14:10 ` [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver Quan Nguyen
  2021-04-02 12:01   ` Philipp Zabel
@ 2021-04-07 15:50   ` Corey Minyard
  2021-04-22 10:49   ` Graeme Gregory
  2 siblings, 0 replies; 13+ messages in thread
From: Corey Minyard @ 2021-04-07 15:50 UTC (permalink / raw)
  To: Quan Nguyen
  Cc: devicetree, linux-aspeed, Andrew Jeffery, openbmc,
	Thang Q . Nguyen, linux-kernel, Phong Vo, Wolfram Sang,
	Rob Herring, Philipp Zabel, openipmi-developer,
	Open Source Submission, linux-arm-kernel, linux-i2c

On Tue, Mar 30, 2021 at 09:10:28PM +0700, Quan Nguyen wrote:
> The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
> in-band IPMI communication with their host in management (BMC) side.
> 
> This commits adds support specifically for Aspeed AST2500 which commonly
> used as Board Management Controllers.

Two major comments:

This needs to be two patches: one with the generic SSIF slave code, and
one with the aspeed-specific code.  It's hard to tell that you are
adding generic code otherwise.

If you are going to add a generic interface like this, you need to add
documentation on how to use it, but from userland and from in the
kernel.

And some other comments:

Did you run this through checkpatch?

I think there is a high-level race condition in this code.  Consider the
following sequence:

  1) host writes a request to the BMC
  2) BMC reads the request
  3) host aborts the operation
  4) host write a new request to the BMC
  5) BMC sends the response to the first message

You probably need something to say that a response can only go out if
there is a request that has come in that has been read by the BMC.

Other comments inline.

> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
>  drivers/char/ipmi/Kconfig           |  22 +
>  drivers/char/ipmi/Makefile          |   2 +
>  drivers/char/ipmi/ssif_bmc.c        | 645 ++++++++++++++++++++++++++++
>  drivers/char/ipmi/ssif_bmc.h        |  92 ++++
>  drivers/char/ipmi/ssif_bmc_aspeed.c | 132 ++++++
>  5 files changed, 893 insertions(+)
>  create mode 100644 drivers/char/ipmi/ssif_bmc.c
>  create mode 100644 drivers/char/ipmi/ssif_bmc.h
>  create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c
> 
> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> index 07847d9a459a..45be57023577 100644
> --- a/drivers/char/ipmi/Kconfig
> +++ b/drivers/char/ipmi/Kconfig
> @@ -133,6 +133,28 @@ config ASPEED_BT_IPMI_BMC
>  	  found on Aspeed SOCs (AST2400 and AST2500). The driver
>  	  implements the BMC side of the BT interface.
>  
> +config SSIF_IPMI_BMC
> +	tristate "SSIF IPMI BMC driver"
> +	select I2C
> +	select I2C_SLAVE
> +	help
> +	  This enables the IPMI SMBus system interface (SSIF) at the
> +	  management (BMC) side.
> +
> +	  The driver implements the BMC side of the SMBus system
> +	  interface (SSIF).
> +
> +config ASPEED_SSIF_IPMI_BMC
> +	depends on ARCH_ASPEED || COMPILE_TEST
> +	select SSIF_IPMI_BMC
> +	tristate "Aspeed SSIF IPMI BMC driver"
> +	help
> +	  Provides a driver for the SSIF IPMI interface found on
> +	  Aspeed AST2500 SoC.
> +
> +	  The driver implements the BMC side of the SMBus system
> +	  interface (SSIF), specific for Aspeed AST2500 SoC.
> +
>  config IPMB_DEVICE_INTERFACE
>  	tristate 'IPMB Interface handler'
>  	depends on I2C
> diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
> index 0822adc2ec41..05b993f7335b 100644
> --- a/drivers/char/ipmi/Makefile
> +++ b/drivers/char/ipmi/Makefile
> @@ -27,3 +27,5 @@ obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
>  obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
>  obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o
>  obj-$(CONFIG_IPMB_DEVICE_INTERFACE) += ipmb_dev_int.o
> +obj-$(CONFIG_SSIF_IPMI_BMC) += ssif_bmc.o
> +obj-$(CONFIG_ASPEED_SSIF_IPMI_BMC) += ssif_bmc_aspeed.o
> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
> new file mode 100644
> index 000000000000..ae6e8750c795
> --- /dev/null
> +++ b/drivers/char/ipmi/ssif_bmc.c
> @@ -0,0 +1,645 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * The driver for BMC side of SSIF interface
> + *
> + * Copyright (c) 2021, Ampere Computing LLC
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <https://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/poll.h>
> +#include <linux/sched.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +
> +#include "ssif_bmc.h"
> +
> +/*
> + * Call in WRITE context
> + */
> +static int send_ssif_bmc_response(struct ssif_bmc_ctx *ssif_bmc, bool non_blocking)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!non_blocking) {
> +retry:
> +		ret = wait_event_interruptible(ssif_bmc->wait_queue,
> +					       !ssif_bmc->response_in_progress);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	if (ssif_bmc->response_in_progress) {
> +		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +		if (non_blocking)
> +			return -EAGAIN;
> +
> +		goto retry;
> +	}

This would be a lot more elegant, and work better, if it was:

	
	spin_lock_irqsave(&ssif_bmc->lock, flags);
	while (ssif_bmc->response_in_progress) {
		if (non_blocking) {
			ret = -EAGAIN;
			goto out_unlock;
		}
		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
		ret = wait_event_interruptible(ssif_bmc->wait_queue,
					       !ssif_bmc->response_in_progress);
		if (ret)
			return ret;
		spin_lock_irqsave(&ssif_bmc->lock, flags);
	}

Same with the receive function.  It avoids calling
wait_event_unnterruptible() unless required, and is easier to read.

> +
> +	/*
> +	 * Check the response data length from userspace to determine the type
> +	 * of the response message whether it is single-part or multi-part.
> +	 */
> +	ssif_bmc->is_singlepart_read =
> +		(ssif_msg_len(&ssif_bmc->response) <= (MAX_PAYLOAD_PER_TRANSACTION + 1)) ?
> +		true : false; /* 1: byte of length */
> +
> +	ssif_bmc->response_in_progress = true;
> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +
> +	return 0;
> +}
> +
> +/*
> + * Call in READ context
> + */
> +static int receive_ssif_bmc_request(struct ssif_bmc_ctx *ssif_bmc, bool non_blocking)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!non_blocking) {
> +retry:
> +		ret = wait_event_interruptible(ssif_bmc->wait_queue,
> +					       ssif_bmc->request_available);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	if (!ssif_bmc->request_available) {
> +		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +		if (non_blocking)
> +			return -EAGAIN;
> +		goto retry;
> +	}
> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +
> +	return 0;
> +}
> +
> +/* Handle SSIF message that will be sent to user */
> +static ssize_t ssif_bmc_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
> +	struct ssif_msg msg;
> +	unsigned long flags;
> +	ssize_t ret;
> +
> +	mutex_lock(&ssif_bmc->file_mutex);

If you are sitting on a blocking read here, there's no way to send a
message, do poll, etc because you use the same mutex.  That doesn't seem
like a good idea.

> +
> +	ret = receive_ssif_bmc_request(ssif_bmc, file->f_flags & O_NONBLOCK);
> +	if (ret < 0)
> +		goto out;

This design has some issues.  I'd recommend getting rid of file_mutex
and only using the spinlock.  Claim the spinlock before calling
receive_ssif_bmc_request(), release the spinlock in that function if you
need to block (and document heavily that you do this), and when it
returns you either have an error or there is a message waiting.

You can actually just get rid of receive_ssif_bmc_request() and inline
the code here, if you like.  That might be easier to read.

> +
> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	count = min_t(ssize_t, count, ssif_msg_len(&ssif_bmc->request));
> +	memcpy(&msg, &ssif_bmc->request, count);
> +	ssif_bmc->request_available = false;
> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +
> +	ret = copy_to_user(buf, &msg, count);
> +out:
> +	mutex_unlock(&ssif_bmc->file_mutex);
> +
> +	return (ret < 0) ? ret : count;
> +}
> +
> +/* Handle SSIF message that is written by user */
> +static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t count,
> +			      loff_t *ppos)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
> +	struct ssif_msg msg;
> +	unsigned long flags;
> +	ssize_t ret;
> +
> +	if (count > sizeof(struct ssif_msg))
> +		return -EINVAL;
> +
> +	mutex_lock(&ssif_bmc->file_mutex);
> +
> +	ret = copy_from_user(&msg, buf, count);
> +	if (ret)
> +		goto out;

Same basic comments as the read side.  There is no need, BTW, to grab
the mutex before calling copy_from_user, since it's going into an
internal message buffer.

Speaking of that, ssif_msg is moderately big.  It's probably ok in this
case, but putting big things on the stack is generally frowned upon.

> +
> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	if (count >= ssif_msg_len(&ssif_bmc->response))
> +		memcpy(&ssif_bmc->response, &msg, count);
> +	else
> +		ret = -EINVAL;
> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +
> +	if (ret)
> +		goto out;
> +
> +	ret = send_ssif_bmc_response(ssif_bmc, file->f_flags & O_NONBLOCK);
> +	if (!ret && ssif_bmc->set_ssif_bmc_status)
> +		ssif_bmc->set_ssif_bmc_status(ssif_bmc, SSIF_BMC_READY);
> +out:
> +	mutex_unlock(&ssif_bmc->file_mutex);
> +
> +	return (ret < 0) ? ret : count;
> +}
> +
> +static long ssif_bmc_ioctl(struct file *file, unsigned int cmd, unsigned long param)
> +{
> +	return 0;

I believe -EINVAL is the right return value here.  Or just remove the
function; it's not necessary if not implemented.

> +}
> +
> +static unsigned int ssif_bmc_poll(struct file *file, poll_table *wait)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
> +	unsigned int mask = 0;
> +
> +	mutex_lock(&ssif_bmc->file_mutex);

With this above comments, this will need to claim the spinlock.  Which
is what you want, anyway, because request_available is set under the
spinlock.

> +	poll_wait(file, &ssif_bmc->wait_queue, wait);
> +
> +	/*
> +	 * The request message is now available so userspace application can
> +	 * get the request
> +	 */
> +	if (ssif_bmc->request_available)
> +		mask |= POLLIN;

Is there a reason you don't implement the write side of this?  That
would seem useful.  Otherwise what's the write side supposed to do if it
gets an EAGAIN from the write()?  I guess, given how this driver works,
that's not really possible if it's used correctly.  But it would provide
a way to know when a response has finished send.

> +
> +	mutex_unlock(&ssif_bmc->file_mutex);
> +	return mask;
> +}
> +
> +/*
> + * System calls to device interface for user apps
> + */
> +static const struct file_operations ssif_bmc_fops = {
> +	.owner		= THIS_MODULE,
> +	.read		= ssif_bmc_read,
> +	.write		= ssif_bmc_write,
> +	.poll		= ssif_bmc_poll,
> +	.unlocked_ioctl	= ssif_bmc_ioctl,
> +};
> +
> +/* Called with ssif_bmc->lock held. */
> +static int handle_request(struct ssif_bmc_ctx *ssif_bmc)
> +{
> +	if (ssif_bmc->set_ssif_bmc_status)
> +		ssif_bmc->set_ssif_bmc_status(ssif_bmc, SSIF_BMC_BUSY);
> +
> +	/* Request message is available to process */
> +	ssif_bmc->request_available = true;
> +	/*
> +	 * This is the new READ request.
> +	 * Clear the response buffer of the previous transaction
> +	 */
> +	memset(&ssif_bmc->response, 0, sizeof(struct ssif_msg));

This memset doesn't seem necessary.  You don't need to clear the data, I
don't think.

> +	wake_up_all(&ssif_bmc->wait_queue);
> +	return 0;
> +}
> +
> +/* Called with ssif_bmc->lock held. */
> +static int complete_response(struct ssif_bmc_ctx *ssif_bmc)
> +{
> +	/* Invalidate response in buffer to denote it having been sent. */
> +	ssif_bmc->response.len = 0;
> +	ssif_bmc->response_in_progress = false;
> +	ssif_bmc->nbytes_processed = 0;
> +	ssif_bmc->remain_len = 0;
> +	memset(&ssif_bmc->response_buf, 0, MAX_PAYLOAD_PER_TRANSACTION);

This memset seems unnecessary.

> +	wake_up_all(&ssif_bmc->wait_queue);
> +	return 0;
> +}
> +
> +static void set_multipart_response_buffer(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
> +{
> +	u8 response_len = 0;
> +	int idx = 0;
> +	u8 data_len;
> +
> +	data_len = ssif_bmc->response.len;
> +	switch (ssif_bmc->smbus_cmd) {
> +	case SSIF_IPMI_MULTIPART_READ_START:
> +		/*
> +		 * Read Start length is 32 bytes.
> +		 * Read Start transfer first 30 bytes of IPMI response
> +		 * and 2 special code 0x00, 0x01.
> +		 */
> +		*val = MAX_PAYLOAD_PER_TRANSACTION;
> +		ssif_bmc->remain_len = data_len - MAX_IPMI_DATA_PER_START_TRANSACTION;
> +		ssif_bmc->block_num = 0;
> +
> +		ssif_bmc->response_buf[idx++] = 0x00; /* Start Flag */
> +		ssif_bmc->response_buf[idx++] = 0x01; /* Start Flag */
> +		ssif_bmc->response_buf[idx++] = ssif_bmc->response.netfn_lun;
> +		ssif_bmc->response_buf[idx++] = ssif_bmc->response.cmd;
> +		ssif_bmc->response_buf[idx++] = ssif_bmc->response.payload[0];
> +
> +		response_len = MAX_PAYLOAD_PER_TRANSACTION - idx;
> +
> +		memcpy(&ssif_bmc->response_buf[idx], &ssif_bmc->response.payload[1],
> +		       response_len);
> +		break;
> +
> +	case SSIF_IPMI_MULTIPART_READ_MIDDLE:
> +		/*
> +		 * IPMI READ Middle or READ End messages can carry up to 31 bytes
> +		 * IPMI data plus block number byte.
> +		 */
> +		if (ssif_bmc->remain_len < MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION) {
> +			/*
> +			 * This is READ End message
> +			 *  Return length is the remaining response data length
> +			 *  plus block number
> +			 *  Block number 0xFF is to indicate this is last message
> +			 *
> +			 * Return length is: remain response plus block number
> +			 */
> +			*val = ssif_bmc->remain_len + 1;
> +			ssif_bmc->block_num = 0xFF;
> +			ssif_bmc->response_buf[idx++] = ssif_bmc->block_num;
> +			response_len = ssif_bmc->remain_len;
> +		} else {
> +			/*
> +			 * This is READ Middle message
> +			 *  Response length is the maximum SMBUS transfer length
> +			 *  Block number byte is incremented
> +			 * Return length is maximum SMBUS transfer length
> +			 */
> +			*val = MAX_PAYLOAD_PER_TRANSACTION;
> +			ssif_bmc->remain_len -= MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION;
> +			response_len = MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION;
> +			ssif_bmc->response_buf[idx++] = ssif_bmc->block_num;
> +			ssif_bmc->block_num++;
> +		}
> +
> +		memcpy(&ssif_bmc->response_buf[idx],
> +		       ssif_bmc->response.payload + 1 + ssif_bmc->nbytes_processed,
> +		       response_len);
> +		break;
> +
> +	default:
> +		/* Do not expect to go to this case */
> +		pr_err("Error: Unexpected SMBus command received 0x%x\n", ssif_bmc->smbus_cmd);
> +		break;
> +	}
> +
> +	ssif_bmc->nbytes_processed += response_len;
> +}
> +
> +static void set_singlepart_response_buffer(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
> +{
> +	u8 *buf = (u8 *)&ssif_bmc->response;
> +
> +	/*
> +	 * Do not expect the IPMI response has data length 0.
> +	 * With some I2C SMBus controllers (Aspeed I2C), return 0 for
> +	 * the SMBus Read Request callback might cause bad state for
> +	 * the bus. So return 1 byte length so that master will
> +	 * resend the Read Request because the length of response is
> +	 * less than a normal IPMI response.
> +	 *
> +	 * Otherwise, return the length of IPMI response

Umm, shouldn't you reject zero length messages from the user, since that
is invalid?  Then this problem would go away.

> +	 */
> +	*val = (buf[ssif_bmc->msg_idx]) ? buf[ssif_bmc->msg_idx] : 0x1;
> +}
> +
> +/* Process the IPMI response that will be read by master */
> +static void handle_read_processed(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
> +{
> +	u8 *buf;
> +	u8 pec_len, addr, len;
> +	u8 pec = 0;
> +
> +	pec_len = ssif_bmc->pec_support ? 1 : 0;
> +	/* PEC - Start Read Address */
> +	addr = GET_8BIT_ADDR(ssif_bmc->client->addr);
> +	pec = i2c_smbus_pec(pec, &addr, 1);
> +	/* PEC - SSIF Command */
> +	pec = i2c_smbus_pec(pec, &ssif_bmc->smbus_cmd, 1);
> +	/* PEC - Restart Write Address */
> +	addr = addr | 0x01;
> +	pec = i2c_smbus_pec(pec, &addr, 1);
> +
> +	if (ssif_bmc->is_singlepart_read) {
> +		/* Single-part Read processing */
> +		buf = (u8 *)&ssif_bmc->response;
> +
> +		if (ssif_bmc->response.len && ssif_bmc->msg_idx < ssif_bmc->response.len) {
> +			ssif_bmc->msg_idx++;
> +			*val = buf[ssif_bmc->msg_idx];
> +		} else if (ssif_bmc->response.len &&
> +			   (ssif_bmc->msg_idx == ssif_bmc->response.len)) {
> +			ssif_bmc->msg_idx++;
> +			*val = i2c_smbus_pec(pec, buf, ssif_msg_len(&ssif_bmc->response));
> +		} else {
> +			*val = 0;
> +		}
> +		/* Invalidate response buffer to denote it is sent */
> +		if (ssif_bmc->msg_idx + 1 >= (ssif_msg_len(&ssif_bmc->response) + pec_len))
> +			complete_response(ssif_bmc);
> +	} else {
> +		/* Multi-part Read processing */
> +		switch (ssif_bmc->smbus_cmd) {
> +		case SSIF_IPMI_MULTIPART_READ_START:
> +		case SSIF_IPMI_MULTIPART_READ_MIDDLE:
> +			buf = (u8 *)&ssif_bmc->response_buf;
> +			*val = buf[ssif_bmc->msg_idx];
> +			ssif_bmc->msg_idx++;
> +			break;
> +		default:
> +			/* Do not expect to go to this case */
> +			pr_err("Error: Unexpected SMBus command received 0x%x\n",
> +			       ssif_bmc->smbus_cmd);
> +			break;
> +		}
> +		len = (ssif_bmc->block_num == 0xFF) ?
> +		       ssif_bmc->remain_len + 1 : MAX_PAYLOAD_PER_TRANSACTION;
> +		if (ssif_bmc->msg_idx == (len + 1)) {
> +			pec = i2c_smbus_pec(pec, &len, 1);
> +			*val = i2c_smbus_pec(pec, ssif_bmc->response_buf, len);
> +		}
> +		/* Invalidate response buffer to denote last response is sent */
> +		if (ssif_bmc->block_num == 0xFF &&
> +		    ssif_bmc->msg_idx > (ssif_bmc->remain_len + pec_len)) {
> +			complete_response(ssif_bmc);
> +		}
> +	}
> +}
> +
> +static void handle_write_received(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
> +{
> +	u8 *buf;
> +	u8 smbus_cmd;
> +
> +	buf = (u8 *)&ssif_bmc->request;
> +	if (ssif_bmc->msg_idx >= sizeof(struct ssif_msg))
> +		return;
> +
> +	smbus_cmd = ssif_bmc->smbus_cmd;
> +	switch (smbus_cmd) {
> +	case SSIF_IPMI_SINGLEPART_WRITE:
> +		/* Single-part write */
> +		buf[ssif_bmc->msg_idx - 1] = *val;
> +		ssif_bmc->msg_idx++;
> +
> +		break;
> +	case SSIF_IPMI_MULTIPART_WRITE_START:
> +		/* Reset length to zero */
> +		if (ssif_bmc->msg_idx == 1)
> +			ssif_bmc->request.len = 0;
> +
> +		fallthrough;
> +	case SSIF_IPMI_MULTIPART_WRITE_MIDDLE:
> +	case SSIF_IPMI_MULTIPART_WRITE_END:
> +		/* Multi-part write, 2nd byte received is length */
> +		if (ssif_bmc->msg_idx == 1) {
> +			ssif_bmc->request.len += *val;
> +			ssif_bmc->recv_len = *val;
> +		} else {
> +			buf[ssif_bmc->msg_idx - 1 +
> +			    ssif_bmc->request.len - ssif_bmc->recv_len]	= *val;
> +		}
> +
> +		ssif_bmc->msg_idx++;
> +
> +		break;
> +	default:
> +		/* Do not expect to go to this case */
> +		pr_err("Error: Unexpected SMBus command received 0x%x\n", ssif_bmc->smbus_cmd);
> +		break;
> +	}
> +}
> +
> +static bool validate_pec(struct ssif_bmc_ctx *ssif_bmc)
> +{
> +	u8 rpec = 0, cpec = 0;
> +	bool ret = true;
> +	u8 addr, index;
> +	u8 *buf;
> +
> +	buf = (u8 *)&ssif_bmc->request;
> +	switch (ssif_bmc->smbus_cmd) {
> +	case SSIF_IPMI_SINGLEPART_WRITE:
> +		if ((ssif_bmc->msg_idx - 1) == ssif_msg_len(&ssif_bmc->request)) {
> +			/* PEC is not included */
> +			ssif_bmc->pec_support = false;
> +			return true;
> +		}
> +
> +		if ((ssif_bmc->msg_idx - 1) != (ssif_msg_len(&ssif_bmc->request) + 1))
> +			goto error;
> +
> +		/* PEC is included */
> +		ssif_bmc->pec_support = true;
> +		rpec = buf[ssif_bmc->msg_idx - 2];
> +		addr = GET_8BIT_ADDR(ssif_bmc->client->addr);
> +		cpec = i2c_smbus_pec(cpec, &addr, 1);
> +		cpec = i2c_smbus_pec(cpec, &ssif_bmc->smbus_cmd, 1);
> +		cpec = i2c_smbus_pec(cpec, buf, ssif_msg_len(&ssif_bmc->request));
> +		if (rpec != cpec) {
> +			pr_err("Bad PEC 0x%02x vs. 0x%02x\n", rpec, cpec);
> +			ret = false;
> +		}
> +
> +		break;
> +	case SSIF_IPMI_MULTIPART_WRITE_START:
> +	case SSIF_IPMI_MULTIPART_WRITE_MIDDLE:
> +	case SSIF_IPMI_MULTIPART_WRITE_END:
> +		index = ssif_bmc->request.len - ssif_bmc->recv_len;
> +		if ((ssif_bmc->msg_idx - 1 + index) == ssif_msg_len(&ssif_bmc->request)) {
> +			/* PEC is not included */
> +			ssif_bmc->pec_support = false;
> +			return true;
> +		}
> +
> +		if ((ssif_bmc->msg_idx - 1 + index) != (ssif_msg_len(&ssif_bmc->request) + 1))
> +			goto error;
> +
> +		/* PEC is included */
> +		ssif_bmc->pec_support = true;
> +		rpec = buf[ssif_bmc->msg_idx - 2 + index];
> +		addr = GET_8BIT_ADDR(ssif_bmc->client->addr);
> +		cpec = i2c_smbus_pec(cpec, &addr, 1);
> +		cpec = i2c_smbus_pec(cpec, &ssif_bmc->smbus_cmd, 1);
> +		cpec = i2c_smbus_pec(cpec, &ssif_bmc->recv_len, 1);
> +		/* As SMBus specification does not allow the length
> +		 * (byte count) in the Write-Block protocol to be zero.
> +		 * Therefore, it is illegal to have the last Middle
> +		 * transaction in the sequence carry 32-bytes and have
> +		 * a length of ‘0’ in the End transaction.
> +		 * But some users may try to use this way and we should
> +		 * prevent ssif_bmc driver broken in this case.
> +		 */
> +		if (ssif_bmc->recv_len != 0)
> +			cpec = i2c_smbus_pec(cpec, buf + 1 + index, ssif_bmc->recv_len);
> +
> +		if (rpec != cpec) {
> +			pr_err("Bad PEC 0x%02x vs. 0x%02x\n", rpec, cpec);
> +			ret = false;
> +		}
> +
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +error:
> +	/* Do not expect to go to this case */
> +	pr_err("Error: Unexpected length received %d\n", ssif_msg_len(&ssif_bmc->request));
> +
> +	return false;
> +}
> +
> +static void complete_write_received(struct ssif_bmc_ctx *ssif_bmc)
> +{
> +	u8 cmd = ssif_bmc->smbus_cmd;
> +
> +	/* A BMC that receives an invalid PEC shall drop the data for the write
> +	 * transaction and any further transactions (read or write) until
> +	 * the next valid read or write Start transaction is received
> +	 */
> +	if (!validate_pec(ssif_bmc)) {
> +		pr_err("Received invalid PEC\n");
> +		return;
> +	}
> +
> +	if (cmd == SSIF_IPMI_SINGLEPART_WRITE || cmd == SSIF_IPMI_MULTIPART_WRITE_END)
> +		handle_request(ssif_bmc);
> +}
> +
> +/*
> + * Callback function to handle I2C slave events
> + */
> +static int ssif_bmc_cb(struct i2c_client *client, enum i2c_slave_event event, u8 *val)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
> +
> +	spin_lock(&ssif_bmc->lock);

You need the _irqsave version here.  You don't know if interrupts are
disabled or not.  Depends on the particular low-level driver.

> +
> +	/* I2C Event Handler:
> +	 *   I2C_SLAVE_READ_REQUESTED	0x0
> +	 *   I2C_SLAVE_WRITE_REQUESTED	0x1
> +	 *   I2C_SLAVE_READ_PROCESSED	0x2
> +	 *   I2C_SLAVE_WRITE_RECEIVED	0x3
> +	 *   I2C_SLAVE_STOP		0x4
> +	 */
> +	switch (event) {
> +	case I2C_SLAVE_READ_REQUESTED:

Shouldn't you NAK if you are expecting a read?

> +		ssif_bmc->msg_idx = 0;
> +		if (ssif_bmc->is_singlepart_read)
> +			set_singlepart_response_buffer(ssif_bmc, val);
> +		else
> +			set_multipart_response_buffer(ssif_bmc, val);
> +		break;
> +
> +	case I2C_SLAVE_WRITE_REQUESTED:

If this happens and you are sending a response, you need to clean up.
In fact, all of these commands need handling if they aren't expected.

> +		ssif_bmc->msg_idx = 0;
> +		break;
> +
> +	case I2C_SLAVE_READ_PROCESSED:
> +		handle_read_processed(ssif_bmc, val);
> +		break;
> +
> +	case I2C_SLAVE_WRITE_RECEIVED:
> +		/*
> +		 * First byte is SMBUS command, not a part of SSIF message.
> +		 * SSIF request buffer starts with msg_idx 1 for the first
> +		 *  buffer byte.
> +		 */
> +		if (ssif_bmc->msg_idx == 0) {
> +			/* SMBUS command can vary (single or multi-part) */
> +			ssif_bmc->smbus_cmd = *val;
> +			ssif_bmc->msg_idx++;
> +		} else {
> +			handle_write_received(ssif_bmc, val);
> +		}
> +
> +		break;
> +
> +	case I2C_SLAVE_STOP:
> +		/*
> +		 * PEC byte is appended at the end of each transaction.
> +		 * Detect PEC is support or not after receiving write request
> +		 * completely.
> +		 */
> +		if (ssif_bmc->last_event == I2C_SLAVE_WRITE_RECEIVED)
> +			complete_write_received(ssif_bmc);
> +		/* Reset message index */
> +		ssif_bmc->msg_idx = 0;
> +		break;
> +
> +	default:
> +		break;
> +	}
> +	ssif_bmc->last_event = event;
> +	spin_unlock(&ssif_bmc->lock);
> +
> +	return 0;
> +}
> +
> +struct ssif_bmc_ctx *ssif_bmc_alloc(struct i2c_client *client, int sizeof_priv)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc;
> +	int ret;
> +
> +	ssif_bmc = devm_kzalloc(&client->dev, sizeof(*ssif_bmc) + sizeof_priv, GFP_KERNEL);
> +	if (!ssif_bmc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	spin_lock_init(&ssif_bmc->lock);
> +
> +	init_waitqueue_head(&ssif_bmc->wait_queue);
> +	ssif_bmc->request_available = false;
> +	ssif_bmc->response_in_progress = false;
> +
> +	mutex_init(&ssif_bmc->file_mutex);
> +
> +	/* Register misc device interface */
> +	ssif_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
> +	ssif_bmc->miscdev.name = DEVICE_NAME;
> +	ssif_bmc->miscdev.fops = &ssif_bmc_fops;
> +	ssif_bmc->miscdev.parent = &client->dev;
> +	ret = misc_register(&ssif_bmc->miscdev);
> +	if (ret)
> +		goto out;
> +
> +	ssif_bmc->client = client;
> +	ssif_bmc->client->flags |= I2C_CLIENT_SLAVE;
> +
> +	/* Register I2C slave */
> +	i2c_set_clientdata(client, ssif_bmc);
> +	ret = i2c_slave_register(client, ssif_bmc_cb);
> +	if (ret) {
> +		misc_deregister(&ssif_bmc->miscdev);
> +		goto out;
> +	}
> +
> +	return ssif_bmc;
> +
> +out:
> +	devm_kfree(&client->dev, ssif_bmc);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(ssif_bmc_alloc);
> +
> +MODULE_AUTHOR("Chuong Tran <chuong@os.amperecomputing.com>");
> +MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
> +MODULE_DESCRIPTION("Linux device driver of the BMC IPMI SSIF interface.");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/char/ipmi/ssif_bmc.h b/drivers/char/ipmi/ssif_bmc.h
> new file mode 100644
> index 000000000000..a2ee090572db
> --- /dev/null
> +++ b/drivers/char/ipmi/ssif_bmc.h
> @@ -0,0 +1,92 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * The driver for BMC side of SSIF interface
> + *
> + * Copyright (c) 2021, Ampere Computing LLC
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <https://www.gnu.org/licenses/>.
> + */
> +#ifndef __SSIF_BMC_H__
> +#define __SSIF_BMC_H__
> +
> +#define DEVICE_NAME				"ipmi-ssif-host"
> +
> +#define GET_8BIT_ADDR(addr_7bit)		(((addr_7bit) << 1) & 0xff)
> +
> +#define MSG_PAYLOAD_LEN_MAX			252
> +
> +/* A standard SMBus Transaction is limited to 32 data bytes */
> +#define MAX_PAYLOAD_PER_TRANSACTION		32
> +
> +#define MAX_IPMI_DATA_PER_START_TRANSACTION	30
> +#define MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION	31
> +
> +#define SSIF_IPMI_SINGLEPART_WRITE		0x2
> +#define SSIF_IPMI_SINGLEPART_READ		0x3
> +#define SSIF_IPMI_MULTIPART_WRITE_START		0x6
> +#define SSIF_IPMI_MULTIPART_WRITE_MIDDLE	0x7
> +#define SSIF_IPMI_MULTIPART_WRITE_END		0x8
> +#define SSIF_IPMI_MULTIPART_READ_START		0x3
> +#define SSIF_IPMI_MULTIPART_READ_MIDDLE		0x9
> +
> +struct ssif_msg {
> +	u8 len;
> +	u8 netfn_lun;
> +	u8 cmd;
> +	u8 payload[MSG_PAYLOAD_LEN_MAX];
> +} __packed;
> +
> +static inline u32 ssif_msg_len(struct ssif_msg *ssif_msg)
> +{
> +	return ssif_msg->len + 1;
> +}
> +
> +#define SSIF_BMC_BUSY   0x01
> +#define SSIF_BMC_READY  0x02
> +
> +struct ssif_bmc_ctx {
> +	struct i2c_client	*client;
> +	struct miscdevice	miscdev;
> +	u8			smbus_cmd;
> +	struct ssif_msg		request;
> +	bool			request_available;
> +	struct ssif_msg		response;
> +	bool			response_in_progress;
> +	/* Response buffer for Multi-part Read Transaction */
> +	u8			response_buf[MAX_PAYLOAD_PER_TRANSACTION];
> +	/* Flag to identify a Multi-part Read Transaction */
> +	bool			is_singlepart_read;
> +	u8			nbytes_processed;
> +	u8			remain_len;
> +	u8			recv_len;
> +	/* Block Number of a Multi-part Read Transaction */
> +	u8			block_num;
> +	size_t			msg_idx;
> +	enum i2c_slave_event	last_event;
> +	bool			pec_support;
> +	spinlock_t		lock;
> +	wait_queue_head_t	wait_queue;
> +	struct mutex		file_mutex;
> +	void (*set_ssif_bmc_status)(struct ssif_bmc_ctx *ssif_bmc, unsigned int flags);
> +	void			*priv;
> +};
> +
> +static inline struct ssif_bmc_ctx *to_ssif_bmc(struct file *file)
> +{
> +	return container_of(file->private_data, struct ssif_bmc_ctx, miscdev);
> +}
> +
> +struct ssif_bmc_ctx *ssif_bmc_alloc(struct i2c_client *client, int sizeof_priv);
> +
> +#endif /* __SSIF_BMC_H__ */
> diff --git a/drivers/char/ipmi/ssif_bmc_aspeed.c b/drivers/char/ipmi/ssif_bmc_aspeed.c
> new file mode 100644
> index 000000000000..a563fcff5acc
> --- /dev/null
> +++ b/drivers/char/ipmi/ssif_bmc_aspeed.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * The driver for BMC side of Aspeed SSIF interface
> + *
> + * Copyright (c) 2021, Ampere Computing LLC
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <https://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/poll.h>
> +#include <linux/iopoll.h>
> +
> +#include "ssif_bmc.h"
> +
> +struct aspeed_i2c_bus {
> +	struct i2c_adapter              adap;
> +	struct device                   *dev;
> +	void __iomem                    *base;
> +	struct reset_control            *rst;
> +	/* Synchronizes I/O mem access to base. */
> +	spinlock_t                      lock;
> +};
> +
> +#define ASPEED_I2C_INTR_CTRL_REG	0x0c
> +#define ASPEED_I2CD_INTR_SLAVE_MATCH	BIT(7)
> +#define ASPEED_I2CD_INTR_RX_DONE	BIT(2)
> +static void aspeed_i2c_enable_interrupt(struct aspeed_i2c_bus *bus, unsigned long mask)
> +{
> +	unsigned long current_mask;
> +
> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +	writel(current_mask | mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +}
> +
> +static void aspeed_i2c_disable_interrupt(struct aspeed_i2c_bus *bus, unsigned long mask)
> +{
> +	unsigned long current_mask;
> +
> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +	writel(current_mask & ~mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);

If there's any other driver that using this register, you are racing
with it.

> +}
> +
> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc, unsigned int status)
> +{
> +	struct aspeed_i2c_bus *bus;
> +	unsigned long flags;
> +
> +	bus = (struct aspeed_i2c_bus *)ssif_bmc->priv;
> +	if (!bus)
> +		return;
> +
> +	spin_lock_irqsave(&bus->lock, flags);

I don't really understand the whole use of this interrupt disable.  What
happens if the host has decided to abort an operation and starts a new
one?  I would think you would want to handle that gracefully.  As it is,
you will sit there waiting for the host to read the response, and it
never will, and you will never get the new transaction.

Plus these don't seem to be interrupts, they seem to be base I2C
disables, which you would really want handled in the I2C driver itself.

I would recommend getting rid of all this and handling a new request
if it comes in.  Then you can just create a generic SSIF BMC compatible
that will work for anything.  Really, if the host has sent a new
message, you need to handle it.

> +
> +	if (status & SSIF_BMC_BUSY) {
> +		/* Ignore RX_DONE and SLAVE_MATCH when slave busy processing */
> +		aspeed_i2c_disable_interrupt(bus, ASPEED_I2CD_INTR_RX_DONE);
> +		aspeed_i2c_disable_interrupt(bus, ASPEED_I2CD_INTR_SLAVE_MATCH);

Why wouldn't you combine these into one call and or the values together?

> +	} else if (status & SSIF_BMC_READY) {
> +		/* Enable RX_DONE and SLAVE_MATCH when slave ready */
> +		aspeed_i2c_enable_interrupt(bus, ASPEED_I2CD_INTR_RX_DONE);
> +		aspeed_i2c_enable_interrupt(bus, ASPEED_I2CD_INTR_SLAVE_MATCH);
> +	}
> +
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +}
> +
> +static int ssif_bmc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc;
> +
> +	ssif_bmc = ssif_bmc_alloc(client, sizeof(struct aspeed_i2c_bus));
> +	if (IS_ERR(ssif_bmc))
> +		return PTR_ERR(ssif_bmc);
> +
> +	ssif_bmc->priv = i2c_get_adapdata(client->adapter);
> +	ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status;
> +
> +	return 0;
> +}
> +
> +static int ssif_bmc_remove(struct i2c_client *client)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
> +
> +	i2c_slave_unregister(client);
> +	misc_deregister(&ssif_bmc->miscdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ssif_bmc_match[] = {
> +	{ .compatible = "aspeed,ast2500-ssif-bmc" },
> +	{ },
> +};
> +
> +static const struct i2c_device_id ssif_bmc_id[] = {
> +	{ DEVICE_NAME, 0 },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, ssif_bmc_id);
> +
> +static struct i2c_driver ssif_bmc_driver = {
> +	.driver		= {
> +		.name		= DEVICE_NAME,
> +		.of_match_table = ssif_bmc_match,
> +	},
> +	.probe		= ssif_bmc_probe,
> +	.remove		= ssif_bmc_remove,
> +	.id_table	= ssif_bmc_id,
> +};
> +
> +module_i2c_driver(ssif_bmc_driver);
> +
> +MODULE_AUTHOR("Chuong Tran <chuong@os.amperecomputing.com>");
> +MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
> +MODULE_DESCRIPTION("Linux device driver of Aspeed BMC IPMI SSIF interface.");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.28.0
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver
  2021-03-30 14:10 ` [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver Quan Nguyen
  2021-04-02 12:01   ` Philipp Zabel
  2021-04-07 15:50   ` Corey Minyard
@ 2021-04-22 10:49   ` Graeme Gregory
  2 siblings, 0 replies; 13+ messages in thread
From: Graeme Gregory @ 2021-04-22 10:49 UTC (permalink / raw)
  To: Quan Nguyen
  Cc: devicetree, linux-aspeed, Corey Minyard, Andrew Jeffery, openbmc,
	Thang Q . Nguyen, linux-kernel, Phong Vo, Wolfram Sang,
	Rob Herring, Philipp Zabel, openipmi-developer,
	Open Source Submission, linux-arm-kernel, linux-i2c

On Tue, Mar 30, 2021 at 09:10:28PM +0700, Quan Nguyen wrote:
> The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
> in-band IPMI communication with their host in management (BMC) side.
> 
> This commits adds support specifically for Aspeed AST2500 which commonly
> used as Board Management Controllers.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
>  drivers/char/ipmi/Kconfig           |  22 +
>  drivers/char/ipmi/Makefile          |   2 +
>  drivers/char/ipmi/ssif_bmc.c        | 645 ++++++++++++++++++++++++++++
>  drivers/char/ipmi/ssif_bmc.h        |  92 ++++
>  drivers/char/ipmi/ssif_bmc_aspeed.c | 132 ++++++
>  5 files changed, 893 insertions(+)
>  create mode 100644 drivers/char/ipmi/ssif_bmc.c
>  create mode 100644 drivers/char/ipmi/ssif_bmc.h
>  create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c
> 
> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> index 07847d9a459a..45be57023577 100644
> --- a/drivers/char/ipmi/Kconfig
> +++ b/drivers/char/ipmi/Kconfig
> @@ -133,6 +133,28 @@ config ASPEED_BT_IPMI_BMC
>  	  found on Aspeed SOCs (AST2400 and AST2500). The driver
>  	  implements the BMC side of the BT interface.
>  
> +config SSIF_IPMI_BMC
> +	tristate "SSIF IPMI BMC driver"
> +	select I2C
> +	select I2C_SLAVE
> +	help
> +	  This enables the IPMI SMBus system interface (SSIF) at the
> +	  management (BMC) side.
> +
> +	  The driver implements the BMC side of the SMBus system
> +	  interface (SSIF).
> +
> +config ASPEED_SSIF_IPMI_BMC
> +	depends on ARCH_ASPEED || COMPILE_TEST
> +	select SSIF_IPMI_BMC
> +	tristate "Aspeed SSIF IPMI BMC driver"
> +	help
> +	  Provides a driver for the SSIF IPMI interface found on
> +	  Aspeed AST2500 SoC.
> +
> +	  The driver implements the BMC side of the SMBus system
> +	  interface (SSIF), specific for Aspeed AST2500 SoC.
> +
>  config IPMB_DEVICE_INTERFACE
>  	tristate 'IPMB Interface handler'
>  	depends on I2C
> diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
> index 0822adc2ec41..05b993f7335b 100644
> --- a/drivers/char/ipmi/Makefile
> +++ b/drivers/char/ipmi/Makefile
> @@ -27,3 +27,5 @@ obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
>  obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
>  obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o
>  obj-$(CONFIG_IPMB_DEVICE_INTERFACE) += ipmb_dev_int.o
> +obj-$(CONFIG_SSIF_IPMI_BMC) += ssif_bmc.o
> +obj-$(CONFIG_ASPEED_SSIF_IPMI_BMC) += ssif_bmc_aspeed.o
> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
> new file mode 100644
> index 000000000000..ae6e8750c795
> --- /dev/null
> +++ b/drivers/char/ipmi/ssif_bmc.c
> @@ -0,0 +1,645 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * The driver for BMC side of SSIF interface
> + *
> + * Copyright (c) 2021, Ampere Computing LLC
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <https://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/poll.h>
> +#include <linux/sched.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +
> +#include "ssif_bmc.h"
> +
> +/*
> + * Call in WRITE context
> + */
> +static int send_ssif_bmc_response(struct ssif_bmc_ctx *ssif_bmc, bool non_blocking)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!non_blocking) {
> +retry:
> +		ret = wait_event_interruptible(ssif_bmc->wait_queue,
> +					       !ssif_bmc->response_in_progress);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	if (ssif_bmc->response_in_progress) {
> +		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +		if (non_blocking)
> +			return -EAGAIN;
> +
> +		goto retry;
> +	}
> +
> +	/*
> +	 * Check the response data length from userspace to determine the type
> +	 * of the response message whether it is single-part or multi-part.
> +	 */
> +	ssif_bmc->is_singlepart_read =
> +		(ssif_msg_len(&ssif_bmc->response) <= (MAX_PAYLOAD_PER_TRANSACTION + 1)) ?
> +		true : false; /* 1: byte of length */
> +
> +	ssif_bmc->response_in_progress = true;
> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +
> +	return 0;
> +}
> +
> +/*
> + * Call in READ context
> + */
> +static int receive_ssif_bmc_request(struct ssif_bmc_ctx *ssif_bmc, bool non_blocking)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!non_blocking) {
> +retry:
> +		ret = wait_event_interruptible(ssif_bmc->wait_queue,
> +					       ssif_bmc->request_available);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	if (!ssif_bmc->request_available) {
> +		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +		if (non_blocking)
> +			return -EAGAIN;
> +		goto retry;
> +	}
> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +
> +	return 0;
> +}
> +
> +/* Handle SSIF message that will be sent to user */
> +static ssize_t ssif_bmc_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
> +	struct ssif_msg msg;
> +	unsigned long flags;
> +	ssize_t ret;
> +
> +	mutex_lock(&ssif_bmc->file_mutex);
> +
> +	ret = receive_ssif_bmc_request(ssif_bmc, file->f_flags & O_NONBLOCK);
> +	if (ret < 0)
> +		goto out;
> +
> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	count = min_t(ssize_t, count, ssif_msg_len(&ssif_bmc->request));
> +	memcpy(&msg, &ssif_bmc->request, count);
> +	ssif_bmc->request_available = false;
> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +
> +	ret = copy_to_user(buf, &msg, count);
> +out:
> +	mutex_unlock(&ssif_bmc->file_mutex);
> +
> +	return (ret < 0) ? ret : count;
> +}
> +
> +/* Handle SSIF message that is written by user */
> +static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t count,
> +			      loff_t *ppos)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
> +	struct ssif_msg msg;
> +	unsigned long flags;
> +	ssize_t ret;
> +
> +	if (count > sizeof(struct ssif_msg))
> +		return -EINVAL;
> +
> +	mutex_lock(&ssif_bmc->file_mutex);
> +
> +	ret = copy_from_user(&msg, buf, count);
> +	if (ret)
> +		goto out;
> +
> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	if (count >= ssif_msg_len(&ssif_bmc->response))
> +		memcpy(&ssif_bmc->response, &msg, count);
> +	else
> +		ret = -EINVAL;
> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +
> +	if (ret)
> +		goto out;
> +
> +	ret = send_ssif_bmc_response(ssif_bmc, file->f_flags & O_NONBLOCK);
> +	if (!ret && ssif_bmc->set_ssif_bmc_status)
> +		ssif_bmc->set_ssif_bmc_status(ssif_bmc, SSIF_BMC_READY);
> +out:
> +	mutex_unlock(&ssif_bmc->file_mutex);
> +
> +	return (ret < 0) ? ret : count;
> +}
> +
> +static long ssif_bmc_ioctl(struct file *file, unsigned int cmd, unsigned long param)
> +{
> +	return 0;
> +}
> +
> +static unsigned int ssif_bmc_poll(struct file *file, poll_table *wait)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
> +	unsigned int mask = 0;
> +
> +	mutex_lock(&ssif_bmc->file_mutex);
> +	poll_wait(file, &ssif_bmc->wait_queue, wait);
> +
> +	/*
> +	 * The request message is now available so userspace application can
> +	 * get the request
> +	 */
> +	if (ssif_bmc->request_available)
> +		mask |= POLLIN;
> +
> +	mutex_unlock(&ssif_bmc->file_mutex);
> +	return mask;
> +}
> +
> +/*
> + * System calls to device interface for user apps
> + */
> +static const struct file_operations ssif_bmc_fops = {
> +	.owner		= THIS_MODULE,
> +	.read		= ssif_bmc_read,
> +	.write		= ssif_bmc_write,
> +	.poll		= ssif_bmc_poll,
> +	.unlocked_ioctl	= ssif_bmc_ioctl,
> +};
> +
> +/* Called with ssif_bmc->lock held. */
> +static int handle_request(struct ssif_bmc_ctx *ssif_bmc)
> +{
> +	if (ssif_bmc->set_ssif_bmc_status)
> +		ssif_bmc->set_ssif_bmc_status(ssif_bmc, SSIF_BMC_BUSY);
> +
> +	/* Request message is available to process */
> +	ssif_bmc->request_available = true;
> +	/*
> +	 * This is the new READ request.
> +	 * Clear the response buffer of the previous transaction
> +	 */
> +	memset(&ssif_bmc->response, 0, sizeof(struct ssif_msg));
> +	wake_up_all(&ssif_bmc->wait_queue);
> +	return 0;
> +}
> +
> +/* Called with ssif_bmc->lock held. */
> +static int complete_response(struct ssif_bmc_ctx *ssif_bmc)
> +{
> +	/* Invalidate response in buffer to denote it having been sent. */
> +	ssif_bmc->response.len = 0;
> +	ssif_bmc->response_in_progress = false;
> +	ssif_bmc->nbytes_processed = 0;
> +	ssif_bmc->remain_len = 0;
> +	memset(&ssif_bmc->response_buf, 0, MAX_PAYLOAD_PER_TRANSACTION);
> +	wake_up_all(&ssif_bmc->wait_queue);
> +	return 0;
> +}
> +
> +static void set_multipart_response_buffer(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
> +{
> +	u8 response_len = 0;
> +	int idx = 0;
> +	u8 data_len;
> +
> +	data_len = ssif_bmc->response.len;
> +	switch (ssif_bmc->smbus_cmd) {
> +	case SSIF_IPMI_MULTIPART_READ_START:
> +		/*
> +		 * Read Start length is 32 bytes.
> +		 * Read Start transfer first 30 bytes of IPMI response
> +		 * and 2 special code 0x00, 0x01.
> +		 */
> +		*val = MAX_PAYLOAD_PER_TRANSACTION;
> +		ssif_bmc->remain_len = data_len - MAX_IPMI_DATA_PER_START_TRANSACTION;
> +		ssif_bmc->block_num = 0;
> +
> +		ssif_bmc->response_buf[idx++] = 0x00; /* Start Flag */
> +		ssif_bmc->response_buf[idx++] = 0x01; /* Start Flag */
> +		ssif_bmc->response_buf[idx++] = ssif_bmc->response.netfn_lun;
> +		ssif_bmc->response_buf[idx++] = ssif_bmc->response.cmd;
> +		ssif_bmc->response_buf[idx++] = ssif_bmc->response.payload[0];
> +
> +		response_len = MAX_PAYLOAD_PER_TRANSACTION - idx;
> +
> +		memcpy(&ssif_bmc->response_buf[idx], &ssif_bmc->response.payload[1],
> +		       response_len);
> +		break;
> +
> +	case SSIF_IPMI_MULTIPART_READ_MIDDLE:
> +		/*
> +		 * IPMI READ Middle or READ End messages can carry up to 31 bytes
> +		 * IPMI data plus block number byte.
> +		 */
> +		if (ssif_bmc->remain_len < MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION) {
> +			/*
> +			 * This is READ End message
> +			 *  Return length is the remaining response data length
> +			 *  plus block number
> +			 *  Block number 0xFF is to indicate this is last message
> +			 *
> +			 * Return length is: remain response plus block number
> +			 */
> +			*val = ssif_bmc->remain_len + 1;
> +			ssif_bmc->block_num = 0xFF;
> +			ssif_bmc->response_buf[idx++] = ssif_bmc->block_num;
> +			response_len = ssif_bmc->remain_len;
> +		} else {
> +			/*
> +			 * This is READ Middle message
> +			 *  Response length is the maximum SMBUS transfer length
> +			 *  Block number byte is incremented
> +			 * Return length is maximum SMBUS transfer length
> +			 */
> +			*val = MAX_PAYLOAD_PER_TRANSACTION;
> +			ssif_bmc->remain_len -= MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION;
> +			response_len = MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION;
> +			ssif_bmc->response_buf[idx++] = ssif_bmc->block_num;
> +			ssif_bmc->block_num++;
> +		}
> +
> +		memcpy(&ssif_bmc->response_buf[idx],
> +		       ssif_bmc->response.payload + 1 + ssif_bmc->nbytes_processed,
> +		       response_len);
> +		break;
> +
> +	default:
> +		/* Do not expect to go to this case */
> +		pr_err("Error: Unexpected SMBus command received 0x%x\n", ssif_bmc->smbus_cmd);
> +		break;
> +	}
> +
> +	ssif_bmc->nbytes_processed += response_len;
> +}
> +
> +static void set_singlepart_response_buffer(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
> +{
> +	u8 *buf = (u8 *)&ssif_bmc->response;
> +
> +	/*
> +	 * Do not expect the IPMI response has data length 0.
> +	 * With some I2C SMBus controllers (Aspeed I2C), return 0 for
> +	 * the SMBus Read Request callback might cause bad state for
> +	 * the bus. So return 1 byte length so that master will
> +	 * resend the Read Request because the length of response is
> +	 * less than a normal IPMI response.
> +	 *
> +	 * Otherwise, return the length of IPMI response
> +	 */
> +	*val = (buf[ssif_bmc->msg_idx]) ? buf[ssif_bmc->msg_idx] : 0x1;
> +}
> +
> +/* Process the IPMI response that will be read by master */
> +static void handle_read_processed(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
> +{
> +	u8 *buf;
> +	u8 pec_len, addr, len;
> +	u8 pec = 0;
> +
> +	pec_len = ssif_bmc->pec_support ? 1 : 0;
> +	/* PEC - Start Read Address */
> +	addr = GET_8BIT_ADDR(ssif_bmc->client->addr);
> +	pec = i2c_smbus_pec(pec, &addr, 1);
> +	/* PEC - SSIF Command */
> +	pec = i2c_smbus_pec(pec, &ssif_bmc->smbus_cmd, 1);
> +	/* PEC - Restart Write Address */
> +	addr = addr | 0x01;
> +	pec = i2c_smbus_pec(pec, &addr, 1);
> +
> +	if (ssif_bmc->is_singlepart_read) {
> +		/* Single-part Read processing */
> +		buf = (u8 *)&ssif_bmc->response;
> +
> +		if (ssif_bmc->response.len && ssif_bmc->msg_idx < ssif_bmc->response.len) {
> +			ssif_bmc->msg_idx++;
> +			*val = buf[ssif_bmc->msg_idx];
> +		} else if (ssif_bmc->response.len &&
> +			   (ssif_bmc->msg_idx == ssif_bmc->response.len)) {
> +			ssif_bmc->msg_idx++;
> +			*val = i2c_smbus_pec(pec, buf, ssif_msg_len(&ssif_bmc->response));
> +		} else {
> +			*val = 0;
> +		}
> +		/* Invalidate response buffer to denote it is sent */
> +		if (ssif_bmc->msg_idx + 1 >= (ssif_msg_len(&ssif_bmc->response) + pec_len))
> +			complete_response(ssif_bmc);
> +	} else {
> +		/* Multi-part Read processing */
> +		switch (ssif_bmc->smbus_cmd) {
> +		case SSIF_IPMI_MULTIPART_READ_START:
> +		case SSIF_IPMI_MULTIPART_READ_MIDDLE:
> +			buf = (u8 *)&ssif_bmc->response_buf;
> +			*val = buf[ssif_bmc->msg_idx];
> +			ssif_bmc->msg_idx++;
> +			break;
> +		default:
> +			/* Do not expect to go to this case */
> +			pr_err("Error: Unexpected SMBus command received 0x%x\n",
> +			       ssif_bmc->smbus_cmd);
> +			break;
> +		}
> +		len = (ssif_bmc->block_num == 0xFF) ?
> +		       ssif_bmc->remain_len + 1 : MAX_PAYLOAD_PER_TRANSACTION;
> +		if (ssif_bmc->msg_idx == (len + 1)) {
> +			pec = i2c_smbus_pec(pec, &len, 1);
> +			*val = i2c_smbus_pec(pec, ssif_bmc->response_buf, len);
> +		}
> +		/* Invalidate response buffer to denote last response is sent */
> +		if (ssif_bmc->block_num == 0xFF &&
> +		    ssif_bmc->msg_idx > (ssif_bmc->remain_len + pec_len)) {
> +			complete_response(ssif_bmc);
> +		}
> +	}
> +}
> +
> +static void handle_write_received(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
> +{
> +	u8 *buf;
> +	u8 smbus_cmd;
> +
> +	buf = (u8 *)&ssif_bmc->request;
> +	if (ssif_bmc->msg_idx >= sizeof(struct ssif_msg))
> +		return;
> +
> +	smbus_cmd = ssif_bmc->smbus_cmd;
> +	switch (smbus_cmd) {
> +	case SSIF_IPMI_SINGLEPART_WRITE:
> +		/* Single-part write */
> +		buf[ssif_bmc->msg_idx - 1] = *val;
> +		ssif_bmc->msg_idx++;
> +
> +		break;
> +	case SSIF_IPMI_MULTIPART_WRITE_START:
> +		/* Reset length to zero */
> +		if (ssif_bmc->msg_idx == 1)
> +			ssif_bmc->request.len = 0;
> +
> +		fallthrough;
> +	case SSIF_IPMI_MULTIPART_WRITE_MIDDLE:
> +	case SSIF_IPMI_MULTIPART_WRITE_END:
> +		/* Multi-part write, 2nd byte received is length */
> +		if (ssif_bmc->msg_idx == 1) {
> +			ssif_bmc->request.len += *val;
> +			ssif_bmc->recv_len = *val;
> +		} else {
> +			buf[ssif_bmc->msg_idx - 1 +
> +			    ssif_bmc->request.len - ssif_bmc->recv_len]	= *val;
> +		}
> +
> +		ssif_bmc->msg_idx++;
> +
> +		break;
> +	default:
> +		/* Do not expect to go to this case */
> +		pr_err("Error: Unexpected SMBus command received 0x%x\n", ssif_bmc->smbus_cmd);
> +		break;
> +	}
> +}
> +
> +static bool validate_pec(struct ssif_bmc_ctx *ssif_bmc)
> +{
> +	u8 rpec = 0, cpec = 0;
> +	bool ret = true;
> +	u8 addr, index;
> +	u8 *buf;
> +
> +	buf = (u8 *)&ssif_bmc->request;
> +	switch (ssif_bmc->smbus_cmd) {
> +	case SSIF_IPMI_SINGLEPART_WRITE:
> +		if ((ssif_bmc->msg_idx - 1) == ssif_msg_len(&ssif_bmc->request)) {
> +			/* PEC is not included */
> +			ssif_bmc->pec_support = false;
> +			return true;
> +		}
> +
> +		if ((ssif_bmc->msg_idx - 1) != (ssif_msg_len(&ssif_bmc->request) + 1))
> +			goto error;
> +
> +		/* PEC is included */
> +		ssif_bmc->pec_support = true;
> +		rpec = buf[ssif_bmc->msg_idx - 2];
> +		addr = GET_8BIT_ADDR(ssif_bmc->client->addr);
> +		cpec = i2c_smbus_pec(cpec, &addr, 1);
> +		cpec = i2c_smbus_pec(cpec, &ssif_bmc->smbus_cmd, 1);
> +		cpec = i2c_smbus_pec(cpec, buf, ssif_msg_len(&ssif_bmc->request));
> +		if (rpec != cpec) {
> +			pr_err("Bad PEC 0x%02x vs. 0x%02x\n", rpec, cpec);
> +			ret = false;
> +		}
> +
> +		break;
> +	case SSIF_IPMI_MULTIPART_WRITE_START:
> +	case SSIF_IPMI_MULTIPART_WRITE_MIDDLE:
> +	case SSIF_IPMI_MULTIPART_WRITE_END:
> +		index = ssif_bmc->request.len - ssif_bmc->recv_len;
> +		if ((ssif_bmc->msg_idx - 1 + index) == ssif_msg_len(&ssif_bmc->request)) {
> +			/* PEC is not included */
> +			ssif_bmc->pec_support = false;
> +			return true;
> +		}
> +
> +		if ((ssif_bmc->msg_idx - 1 + index) != (ssif_msg_len(&ssif_bmc->request) + 1))
> +			goto error;
> +
> +		/* PEC is included */
> +		ssif_bmc->pec_support = true;
> +		rpec = buf[ssif_bmc->msg_idx - 2 + index];
> +		addr = GET_8BIT_ADDR(ssif_bmc->client->addr);
> +		cpec = i2c_smbus_pec(cpec, &addr, 1);
> +		cpec = i2c_smbus_pec(cpec, &ssif_bmc->smbus_cmd, 1);
> +		cpec = i2c_smbus_pec(cpec, &ssif_bmc->recv_len, 1);
> +		/* As SMBus specification does not allow the length
> +		 * (byte count) in the Write-Block protocol to be zero.
> +		 * Therefore, it is illegal to have the last Middle
> +		 * transaction in the sequence carry 32-bytes and have
> +		 * a length of ‘0’ in the End transaction.
> +		 * But some users may try to use this way and we should
> +		 * prevent ssif_bmc driver broken in this case.
> +		 */
> +		if (ssif_bmc->recv_len != 0)
> +			cpec = i2c_smbus_pec(cpec, buf + 1 + index, ssif_bmc->recv_len);
> +
> +		if (rpec != cpec) {
> +			pr_err("Bad PEC 0x%02x vs. 0x%02x\n", rpec, cpec);
> +			ret = false;
> +		}
> +
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +error:
> +	/* Do not expect to go to this case */
> +	pr_err("Error: Unexpected length received %d\n", ssif_msg_len(&ssif_bmc->request));
> +
> +	return false;
> +}
> +
> +static void complete_write_received(struct ssif_bmc_ctx *ssif_bmc)
> +{
> +	u8 cmd = ssif_bmc->smbus_cmd;
> +
> +	/* A BMC that receives an invalid PEC shall drop the data for the write
> +	 * transaction and any further transactions (read or write) until
> +	 * the next valid read or write Start transaction is received
> +	 */
> +	if (!validate_pec(ssif_bmc)) {
> +		pr_err("Received invalid PEC\n");
> +		return;
> +	}
> +
> +	if (cmd == SSIF_IPMI_SINGLEPART_WRITE || cmd == SSIF_IPMI_MULTIPART_WRITE_END)
> +		handle_request(ssif_bmc);
> +}
> +
> +/*
> + * Callback function to handle I2C slave events
> + */
> +static int ssif_bmc_cb(struct i2c_client *client, enum i2c_slave_event event, u8 *val)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
> +
> +	spin_lock(&ssif_bmc->lock);
> +
> +	/* I2C Event Handler:
> +	 *   I2C_SLAVE_READ_REQUESTED	0x0
> +	 *   I2C_SLAVE_WRITE_REQUESTED	0x1
> +	 *   I2C_SLAVE_READ_PROCESSED	0x2
> +	 *   I2C_SLAVE_WRITE_RECEIVED	0x3
> +	 *   I2C_SLAVE_STOP		0x4
> +	 */
> +	switch (event) {
> +	case I2C_SLAVE_READ_REQUESTED:
> +		ssif_bmc->msg_idx = 0;
> +		if (ssif_bmc->is_singlepart_read)
> +			set_singlepart_response_buffer(ssif_bmc, val);
> +		else
> +			set_multipart_response_buffer(ssif_bmc, val);
> +		break;
> +
> +	case I2C_SLAVE_WRITE_REQUESTED:
> +		ssif_bmc->msg_idx = 0;
> +		break;
> +
> +	case I2C_SLAVE_READ_PROCESSED:
> +		handle_read_processed(ssif_bmc, val);
> +		break;
> +
> +	case I2C_SLAVE_WRITE_RECEIVED:
> +		/*
> +		 * First byte is SMBUS command, not a part of SSIF message.
> +		 * SSIF request buffer starts with msg_idx 1 for the first
> +		 *  buffer byte.
> +		 */
> +		if (ssif_bmc->msg_idx == 0) {
> +			/* SMBUS command can vary (single or multi-part) */
> +			ssif_bmc->smbus_cmd = *val;
> +			ssif_bmc->msg_idx++;
> +		} else {
> +			handle_write_received(ssif_bmc, val);
> +		}
> +
> +		break;
> +
> +	case I2C_SLAVE_STOP:
> +		/*
> +		 * PEC byte is appended at the end of each transaction.
> +		 * Detect PEC is support or not after receiving write request
> +		 * completely.
> +		 */
> +		if (ssif_bmc->last_event == I2C_SLAVE_WRITE_RECEIVED)
> +			complete_write_received(ssif_bmc);
> +		/* Reset message index */
> +		ssif_bmc->msg_idx = 0;
> +		break;
> +
> +	default:
> +		break;
> +	}
> +	ssif_bmc->last_event = event;
> +	spin_unlock(&ssif_bmc->lock);
> +
> +	return 0;
> +}
> +
> +struct ssif_bmc_ctx *ssif_bmc_alloc(struct i2c_client *client, int sizeof_priv)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc;
> +	int ret;
> +
> +	ssif_bmc = devm_kzalloc(&client->dev, sizeof(*ssif_bmc) + sizeof_priv, GFP_KERNEL);
> +	if (!ssif_bmc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	spin_lock_init(&ssif_bmc->lock);
> +
> +	init_waitqueue_head(&ssif_bmc->wait_queue);
> +	ssif_bmc->request_available = false;
> +	ssif_bmc->response_in_progress = false;
> +
> +	mutex_init(&ssif_bmc->file_mutex);
> +
> +	/* Register misc device interface */
> +	ssif_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
> +	ssif_bmc->miscdev.name = DEVICE_NAME;
> +	ssif_bmc->miscdev.fops = &ssif_bmc_fops;
> +	ssif_bmc->miscdev.parent = &client->dev;
> +	ret = misc_register(&ssif_bmc->miscdev);
> +	if (ret)
> +		goto out;
> +
> +	ssif_bmc->client = client;
> +	ssif_bmc->client->flags |= I2C_CLIENT_SLAVE;
> +
> +	/* Register I2C slave */
> +	i2c_set_clientdata(client, ssif_bmc);
> +	ret = i2c_slave_register(client, ssif_bmc_cb);
> +	if (ret) {
> +		misc_deregister(&ssif_bmc->miscdev);
> +		goto out;
> +	}
> +
> +	return ssif_bmc;
> +
> +out:
> +	devm_kfree(&client->dev, ssif_bmc);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(ssif_bmc_alloc);
> +
> +MODULE_AUTHOR("Chuong Tran <chuong@os.amperecomputing.com>");
> +MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
> +MODULE_DESCRIPTION("Linux device driver of the BMC IPMI SSIF interface.");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/char/ipmi/ssif_bmc.h b/drivers/char/ipmi/ssif_bmc.h
> new file mode 100644
> index 000000000000..a2ee090572db
> --- /dev/null
> +++ b/drivers/char/ipmi/ssif_bmc.h
> @@ -0,0 +1,92 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * The driver for BMC side of SSIF interface
> + *
> + * Copyright (c) 2021, Ampere Computing LLC
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <https://www.gnu.org/licenses/>.
> + */
> +#ifndef __SSIF_BMC_H__
> +#define __SSIF_BMC_H__
> +
> +#define DEVICE_NAME				"ipmi-ssif-host"
> +
> +#define GET_8BIT_ADDR(addr_7bit)		(((addr_7bit) << 1) & 0xff)
> +
> +#define MSG_PAYLOAD_LEN_MAX			252
> +
> +/* A standard SMBus Transaction is limited to 32 data bytes */
> +#define MAX_PAYLOAD_PER_TRANSACTION		32
> +
> +#define MAX_IPMI_DATA_PER_START_TRANSACTION	30
> +#define MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION	31
> +
> +#define SSIF_IPMI_SINGLEPART_WRITE		0x2
> +#define SSIF_IPMI_SINGLEPART_READ		0x3
> +#define SSIF_IPMI_MULTIPART_WRITE_START		0x6
> +#define SSIF_IPMI_MULTIPART_WRITE_MIDDLE	0x7
> +#define SSIF_IPMI_MULTIPART_WRITE_END		0x8
> +#define SSIF_IPMI_MULTIPART_READ_START		0x3
> +#define SSIF_IPMI_MULTIPART_READ_MIDDLE		0x9
> +
> +struct ssif_msg {
> +	u8 len;
> +	u8 netfn_lun;
> +	u8 cmd;
> +	u8 payload[MSG_PAYLOAD_LEN_MAX];
> +} __packed;
> +
> +static inline u32 ssif_msg_len(struct ssif_msg *ssif_msg)
> +{
> +	return ssif_msg->len + 1;
> +}
> +
> +#define SSIF_BMC_BUSY   0x01
> +#define SSIF_BMC_READY  0x02
> +
> +struct ssif_bmc_ctx {
> +	struct i2c_client	*client;
> +	struct miscdevice	miscdev;
> +	u8			smbus_cmd;
> +	struct ssif_msg		request;
> +	bool			request_available;
> +	struct ssif_msg		response;
> +	bool			response_in_progress;
> +	/* Response buffer for Multi-part Read Transaction */
> +	u8			response_buf[MAX_PAYLOAD_PER_TRANSACTION];
> +	/* Flag to identify a Multi-part Read Transaction */
> +	bool			is_singlepart_read;
> +	u8			nbytes_processed;
> +	u8			remain_len;
> +	u8			recv_len;
> +	/* Block Number of a Multi-part Read Transaction */
> +	u8			block_num;
> +	size_t			msg_idx;
> +	enum i2c_slave_event	last_event;
> +	bool			pec_support;
> +	spinlock_t		lock;
> +	wait_queue_head_t	wait_queue;
> +	struct mutex		file_mutex;
> +	void (*set_ssif_bmc_status)(struct ssif_bmc_ctx *ssif_bmc, unsigned int flags);
> +	void			*priv;
> +};
> +
> +static inline struct ssif_bmc_ctx *to_ssif_bmc(struct file *file)
> +{
> +	return container_of(file->private_data, struct ssif_bmc_ctx, miscdev);
> +}
> +
> +struct ssif_bmc_ctx *ssif_bmc_alloc(struct i2c_client *client, int sizeof_priv);
> +
> +#endif /* __SSIF_BMC_H__ */
> diff --git a/drivers/char/ipmi/ssif_bmc_aspeed.c b/drivers/char/ipmi/ssif_bmc_aspeed.c
> new file mode 100644
> index 000000000000..a563fcff5acc
> --- /dev/null
> +++ b/drivers/char/ipmi/ssif_bmc_aspeed.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * The driver for BMC side of Aspeed SSIF interface
> + *
> + * Copyright (c) 2021, Ampere Computing LLC
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <https://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/poll.h>
> +#include <linux/iopoll.h>
> +
> +#include "ssif_bmc.h"
> +
> +struct aspeed_i2c_bus {
> +	struct i2c_adapter              adap;
> +	struct device                   *dev;
> +	void __iomem                    *base;
> +	struct reset_control            *rst;
> +	/* Synchronizes I/O mem access to base. */
> +	spinlock_t                      lock;
> +};
> +
> +#define ASPEED_I2C_INTR_CTRL_REG	0x0c
> +#define ASPEED_I2CD_INTR_SLAVE_MATCH	BIT(7)
> +#define ASPEED_I2CD_INTR_RX_DONE	BIT(2)
> +static void aspeed_i2c_enable_interrupt(struct aspeed_i2c_bus *bus, unsigned long mask)
> +{
> +	unsigned long current_mask;
> +
> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +	writel(current_mask | mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +}
> +
> +static void aspeed_i2c_disable_interrupt(struct aspeed_i2c_bus *bus, unsigned long mask)
> +{
> +	unsigned long current_mask;
> +
> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +	writel(current_mask & ~mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +}
> +
> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc, unsigned int status)
> +{
> +	struct aspeed_i2c_bus *bus;
> +	unsigned long flags;
> +
> +	bus = (struct aspeed_i2c_bus *)ssif_bmc->priv;
> +	if (!bus)
> +		return;
> +
> +	spin_lock_irqsave(&bus->lock, flags);

Trying to take this spinlock cannot succeed as its already taken by this
point.

The command sent over I2C to trigger this is

20 02 0C 2C 02 AE 00 00 00 00 00 00 00 00 00 46

CONFIG_SPINLOCK_DEBUG output below. Obviously this deadlocks the kernel.

root@nuviamachine1:~# [   38.790031] BUG: spinlock recursion on CPU#0,
swapper/0/0
[   38.796085]  lock: 0x8114c33c, .magic: dead4ead, .owner: swapper/0/0,
.owner_cpu: 0
[   38.804637] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
5.10.30-51e4d5a-dirty-601c30e-00164-g463ecaa957f0 #7
[   38.815410] Hardware name: Generic DT based system
[   38.820756] Backtrace: 
[   38.823502] [<808e9408>] (dump_backtrace) from [<808e9664>]
(show_stack+0x20/0x24)
[   38.831955]  r7:855362a0 r6:600f0193 r5:00000000 r4:80ca3590
[   38.838277] [<808e9644>] (show_stack) from [<808f45b4>]
(dump_stack+0xa0/0xb4)
[   38.846342] [<808f4514>] (dump_stack) from [<808ea418>]
(spin_dump+0x90/0x98)
[   38.854307]  r7:855362a0 r6:8114c33c r5:80c10400 r4:8114c33c
[   38.860627] [<808ea388>] (spin_dump) from [<80171044>]
(do_raw_spin_lock+0x12c/0x130)
[   38.869365]  r5:ffffe000 r4:8114c33c
[   38.873360] [<80170f18>] (do_raw_spin_lock) from [<808fb628>]
(_raw_spin_lock_irqsave+0x20/0x28)
[   38.883165]  r5:00000001 r4:a00f0193
[   38.887162] [<808fb608>] (_raw_spin_lock_irqsave) from [<80513648>]
(aspeed_set_ssif_bmc_status+0x30/0xbc)
[   38.897937]  r5:00000001 r4:8114c040
[   38.901931] [<80513618>] (aspeed_set_ssif_bmc_status) from
[<80512b00>] (ssif_bmc_cb+0x1c4/0x698)
[   38.911835]  r7:855362a0 r6:8553606c r5:00000004 r4:85536040
[   38.918158] [<8051293c>] (ssif_bmc_cb) from [<8067e648>]
(aspeed_i2c_slave_irq+0xb8/0x264)
[   38.927386]  r10:80c00000 r9:80c0a514 r8:00000010 r7:85545000
r6:00000010 r5:00000010
[   38.936122]  r4:8114c040
[   38.938952] [<8067e590>] (aspeed_i2c_slave_irq) from [<8067ed68>]
(aspeed_i2c_bus_irq+0xd0/0x154)
[   38.948855]  r7:8114c33c r6:00000010 r5:811e106c r4:8114c040
[   38.955175] [<8067ec98>] (aspeed_i2c_bus_irq) from [<8017ae3c>]
(__handle_irq_event_percpu+0x68/0x204)
[   38.965568]  r10:80c00000 r9:80c0a514 r8:00000030 r7:80c01e20
r6:00000000 r5:811e106c
[   38.974305]  r4:855468c0 r3:8067ec98
[   38.978299] [<8017add4>] (__handle_irq_event_percpu) from
[<8017b018>] (handle_irq_event_percpu+0x40/0xa0)
[   38.989077]  r10:10c5387d r9:80c00000 r8:81042800 r7:00000000
r6:811e106c r5:811e106c
[   38.997814]  r4:811e1000
[   39.000644] [<8017afd8>] (handle_irq_event_percpu) from [<8017b0c0>]
(handle_irq_event+0x48/0x6c)
[   39.010546]  r5:811e106c r4:811e1000
[   39.014539] [<8017b078>] (handle_irq_event) from [<80180030>]
(handle_fasteoi_irq+0xc8/0x170)
[   39.024056]  r7:00000000 r6:80c0ac14 r5:811e106c r4:811e1000
[   39.030376] [<8017ff68>] (handle_fasteoi_irq) from [<8017a428>]
(generic_handle_irq+0x40/0x54)
[   39.039989]  r7:00000000 r6:00000001 r5:00000000 r4:80b6c80c
[   39.046312] [<8017a3e8>] (generic_handle_irq) from [<8017a4a8>]
(__handle_domain_irq+0x6c/0xc0)
[   39.056028] [<8017a43c>] (__handle_domain_irq) from [<8010134c>]
(gic_handle_irq+0x7c/0x90)
[   39.065353]  r9:80c00000 r8:bf80200c r7:80b6bdcc r6:bf802000
r5:80c01ee0 r4:80c0ac14
[   39.073998] [<801012d0>] (gic_handle_irq) from [<80100b0c>]
(__irq_svc+0x6c/0x90)
[   39.082340] Exception stack(0x80c01ee0 to 0x80c01f28)
[   39.087981] 1ee0: 00000000 00009a14 bd7dc484 8011a740 80c00000
00000000 80c0a514 80c0a550
[   39.097111] 1f00: 80ca7f5b 80a0ea58 10c5387d 80c01f3c 80c01f40
80c01f30 8010907c 80109080
[   39.106237] 1f20: 600f0013 ffffffff
[   39.110130]  r9:80c00000 r8:80ca7f5b r7:80c01f14 r6:ffffffff
r5:600f0013 r4:80109080
[   39.118773] [<80109038>] (arch_cpu_idle) from [<808fb4c8>]
(default_idle_call+0x38/0x108)
[   39.127907] [<808fb490>] (default_idle_call) from [<80156128>]
(do_idle+0xe4/0x150)
[   39.136457] [<80156044>] (do_idle) from [<80156478>]
(cpu_startup_entry+0x28/0x2c)
[   39.144908]  r9:8ffff564 r8:80b3ba6c r7:00000000 r6:80cc7000
r5:00000001 r4:000000d8
[   39.153545] [<80156450>] (cpu_startup_entry) from [<808f4f10>]
(rest_init+0xbc/0xc4)
[   39.162192] [<808f4e54>] (rest_init) from [<80b00c94>]
(arch_call_rest_init+0x18/0x1c)
[   39.171027]  r5:00000001 r4:80cc7040
[   39.175017] [<80b00c7c>] (arch_call_rest_init) from [<80b0124c>]
(start_kernel+0x53c/0x584)
[   39.184330] [<80b00d10>] (start_kernel) from [<00000000>] (0x0)


> +
> +	if (status & SSIF_BMC_BUSY) {
> +		/* Ignore RX_DONE and SLAVE_MATCH when slave busy processing */
> +		aspeed_i2c_disable_interrupt(bus, ASPEED_I2CD_INTR_RX_DONE);
> +		aspeed_i2c_disable_interrupt(bus, ASPEED_I2CD_INTR_SLAVE_MATCH);
> +	} else if (status & SSIF_BMC_READY) {
> +		/* Enable RX_DONE and SLAVE_MATCH when slave ready */
> +		aspeed_i2c_enable_interrupt(bus, ASPEED_I2CD_INTR_RX_DONE);
> +		aspeed_i2c_enable_interrupt(bus, ASPEED_I2CD_INTR_SLAVE_MATCH);
> +	}
> +
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +}
> +
> +static int ssif_bmc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc;
> +
> +	ssif_bmc = ssif_bmc_alloc(client, sizeof(struct aspeed_i2c_bus));
> +	if (IS_ERR(ssif_bmc))
> +		return PTR_ERR(ssif_bmc);
> +
> +	ssif_bmc->priv = i2c_get_adapdata(client->adapter);
> +	ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status;
> +
> +	return 0;
> +}
> +
> +static int ssif_bmc_remove(struct i2c_client *client)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
> +
> +	i2c_slave_unregister(client);
> +	misc_deregister(&ssif_bmc->miscdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ssif_bmc_match[] = {
> +	{ .compatible = "aspeed,ast2500-ssif-bmc" },
> +	{ },
> +};
> +
> +static const struct i2c_device_id ssif_bmc_id[] = {
> +	{ DEVICE_NAME, 0 },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, ssif_bmc_id);
> +
> +static struct i2c_driver ssif_bmc_driver = {
> +	.driver		= {
> +		.name		= DEVICE_NAME,
> +		.of_match_table = ssif_bmc_match,
> +	},
> +	.probe		= ssif_bmc_probe,
> +	.remove		= ssif_bmc_remove,
> +	.id_table	= ssif_bmc_id,
> +};
> +
> +module_i2c_driver(ssif_bmc_driver);
> +
> +MODULE_AUTHOR("Chuong Tran <chuong@os.amperecomputing.com>");
> +MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
> +MODULE_DESCRIPTION("Linux device driver of Aspeed BMC IPMI SSIF interface.");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.28.0
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-04-22 10:50 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-30 14:10 [PATCH v2 0/3] Add Aspeed SSIF BMC driver Quan Nguyen
2021-03-30 14:10 ` [PATCH v2 1/3] i2c: i2c-core-smbus: Expose PEC calculate function for generic use Quan Nguyen
2021-03-30 14:10 ` [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver Quan Nguyen
2021-04-02 12:01   ` Philipp Zabel
2021-04-07  7:23     ` Quan Nguyen
2021-04-07 15:50   ` Corey Minyard
2021-04-22 10:49   ` Graeme Gregory
2021-03-30 14:10 ` [PATCH v2 3/3] bindings: ipmi: Add binding for " Quan Nguyen
2021-04-01 17:09   ` Rob Herring
2021-04-02  1:59     ` Quan Nguyen
2021-04-02 14:21 ` [PATCH v2 0/3] Add " Corey Minyard
2021-04-07 13:09   ` Quan Nguyen
2021-04-07 14:28     ` Corey Minyard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).