[PATCH v3 0/7] Add Aspeed SSIF BMC driver

[PATCH v3 0/7] Add Aspeed SSIF BMC driver

[Quan Nguyen]

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.

v3:
  + Switched binding doc to use DT schema format [Rob]
  + Splited into generic ssif_bmc and aspeed-specific [Corey, Joel]
  + Removed redundant license info [Joel]
  + Switched to use traditional if-else [Joel]
  + Removed unused ssif_bmc_ioctl() [Joel]
  + Made handle_request()/complete_response() to return void [Joel]
  + Refactored send_ssif_bmc_response()/receive_ssif_bmc_request() [Corey]
  + Remove mutex [Corey]
  + Use spin_lock/unlock_irqsave/restore in callback [Corey]
  + Removed the unnecessary memset [Corey]
  + Switch to use dev_err() [Corey]
  + Combine mask/unmask two interrupts together [Corey]
  + Fixed unhandled Tx done with NAK [Quan]
  + Late ack'ed Tx done w/wo Ack irq [Quan]
  + Use aspeed-specific exported aspeed_set_slave_busy() when slave busy
to fix the deadlock [Graeme, Philipp, Quan]
  + Clean buffer for last multipart read [Quan]
  + Handle unknown incoming command [Quan]

v2:
  + Fixed compiling error with COMPILE_TEST for arc

Quan Nguyen (7):
  i2c: i2c-core-smbus: Expose PEC calculate function for generic use
  ipmi: ssif_bmc: Add SSIF BMC driver
  i2c: aspeed: Fix unhandled Tx done with NAK
  i2c: aspeed: Acknowledge Tx done w/wo ACK irq late
  i2c: aspeed: Add aspeed_set_slave_busy()
  ipmi: ssif_bmc: Add Aspeed SSIF BMC driver
  bindings: ipmi: Add binding for Aspeed SSIF BMC driver

 .../bindings/ipmi/aspeed-ssif-bmc.yaml        |  33 +
 drivers/char/ipmi/Kconfig                     |  22 +
 drivers/char/ipmi/Makefile                    |   2 +
 drivers/char/ipmi/ssif_bmc.c                  | 605 ++++++++++++++++++
 drivers/char/ipmi/ssif_bmc.h                  |  93 +++
 drivers/char/ipmi/ssif_bmc_aspeed.c           |  75 +++
 drivers/i2c/busses/i2c-aspeed.c               |  51 +-
 drivers/i2c/i2c-core-smbus.c                  |  12 +-
 include/linux/i2c.h                           |   1 +
 9 files changed, 884 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
 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

[PATCH v3 1/7] i2c: i2c-core-smbus: Expose PEC calculate function for generic use

[Quan Nguyen]

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 e8f2ac8c9c3d..2494a007dfda 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -147,6 +147,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

Re: [PATCH v3 1/7] i2c: i2c-core-smbus: Expose PEC calculate function for generic use

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 375 bytes --]

On Wed, May 19, 2021 at 02:49:28PM +0700, Quan Nguyen wrote:
> Expose the PEC calculation i2c_smbus_pec() for generic use.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

You are the second one who wanted this exported. I agree it makes sense
for slave drivers. So, I'll skip the required user because two are in
flight. Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 2/7] ipmi: ssif_bmc: Add SSIF BMC driver

[Quan Nguyen]

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

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
v3:
  + Removed redundant license info [Joel]
  + Switched to use traditional if-else [Joel]
  + Removed unused ssif_bmc_ioctl() [Joel]
  + Made handle_request()/complete_response() to return void [Joel]
  + Refactored send_ssif_bmc_response()/receive_ssif_bmc_request() [Corey]
  + Removed mutex [Corey]
  + Use spin_lock/unlock_irqsave/restore in callback [Corey]
  + Removed the unnecessary memset [Corey]
  + Switch to use dev_err() [Corey]

v2:
  + Fixed compiling error with COMPILE_TEST for arc

 drivers/char/ipmi/Kconfig    |  11 +
 drivers/char/ipmi/Makefile   |   1 +
 drivers/char/ipmi/ssif_bmc.c | 605 +++++++++++++++++++++++++++++++++++
 drivers/char/ipmi/ssif_bmc.h |  93 ++++++
 4 files changed, 710 insertions(+)
 create mode 100644 drivers/char/ipmi/ssif_bmc.c
 create mode 100644 drivers/char/ipmi/ssif_bmc.h

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 07847d9a459a..ad5c5161bcd6 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -133,6 +133,17 @@ 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 IPMB_DEVICE_INTERFACE
 	tristate 'IPMB Interface handler'
 	depends on I2C
diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
index 0822adc2ec41..d04a214d74c4 100644
--- a/drivers/char/ipmi/Makefile
+++ b/drivers/char/ipmi/Makefile
@@ -27,3 +27,4 @@ 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
diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
new file mode 100644
index 000000000000..d0ea7059b1db
--- /dev/null
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -0,0 +1,605 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * The driver for BMC side of SSIF interface
+ *
+ * Copyright (c) 2021, Ampere Computing LLC
+ *
+ */
+
+#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"
+
+/* 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;
+
+	spin_lock_irqsave(&ssif_bmc->lock, flags);
+	while (!ssif_bmc->request_available) {
+		if (file->f_flags & O_NONBLOCK) {
+			ret = -EAGAIN;
+			goto out_unlock;
+		}
+		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+		ret = wait_event_interruptible(ssif_bmc->wait_queue,
+					       ssif_bmc->request_available);
+		if (ret)
+			return ret;
+		spin_lock_irqsave(&ssif_bmc->lock, flags);
+	}
+
+	count = min_t(ssize_t, ssif_msg_len(&ssif_bmc->request),
+		      min_t(ssize_t, sizeof(struct ssif_msg), count));
+	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);
+	goto out;
+
+out_unlock:
+	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+out:
+	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;
+
+	ret = copy_from_user(&msg, buf, count);
+	if (ret)
+		return ret;
+
+	if (!msg.len || count < ssif_msg_len(&msg))
+		return -EINVAL;
+
+	spin_lock_irqsave(&ssif_bmc->lock, flags);
+	while (ssif_bmc->response_in_progress) {
+		if (file->f_flags & O_NONBLOCK) {
+			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)
+			goto out;
+		spin_lock_irqsave(&ssif_bmc->lock, flags);
+	}
+
+	memcpy(&ssif_bmc->response, &msg, count);
+	ssif_bmc->is_singlepart_read = (ssif_msg_len(&msg) <= MAX_PAYLOAD_PER_TRANSACTION + 1);
+	ssif_bmc->response_in_progress = true;
+	if (ssif_bmc->set_ssif_bmc_status)
+		ssif_bmc->set_ssif_bmc_status(ssif_bmc, SSIF_BMC_READY);
+
+out_unlock:
+	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+
+out:
+	return (ret < 0) ? ret : count;
+}
+
+static int ssif_bmc_open(struct inode *inode, struct file *file)
+{
+	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
+	int ret = 0;
+
+	spin_lock_irq(&ssif_bmc->lock);
+	if (!ssif_bmc->running)
+		ssif_bmc->running = 1;
+	else
+		ret = -EBUSY;
+	spin_unlock_irq(&ssif_bmc->lock);
+
+	return ret;
+}
+
+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;
+
+	poll_wait(file, &ssif_bmc->wait_queue, wait);
+
+	spin_lock_irq(&ssif_bmc->lock);
+	/*
+	 * The request message is now available so userspace application can
+	 * get the request
+	 */
+	if (ssif_bmc->request_available)
+		mask |= POLLIN;
+
+	spin_unlock_irq(&ssif_bmc->lock);
+
+	return mask;
+}
+
+static int ssif_bmc_release(struct inode *inode, struct file *file)
+{
+	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
+
+	spin_lock_irq(&ssif_bmc->lock);
+	ssif_bmc->running = 0;
+	spin_unlock_irq(&ssif_bmc->lock);
+
+	return 0;
+}
+
+/*
+ * System calls to device interface for user apps
+ */
+static const struct file_operations ssif_bmc_fops = {
+	.owner		= THIS_MODULE,
+	.open		= ssif_bmc_open,
+	.read		= ssif_bmc_read,
+	.write		= ssif_bmc_write,
+	.release	= ssif_bmc_release,
+	.poll		= ssif_bmc_poll,
+};
+
+/* Called with ssif_bmc->lock held. */
+static void 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.
+	 */
+	wake_up_all(&ssif_bmc->wait_queue);
+}
+
+/* Called with ssif_bmc->lock held. */
+static void 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;
+	wake_up_all(&ssif_bmc->wait_queue);
+}
+
+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
+			 *
+			 */
+			*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;
+			/* Clean the buffer */
+			memset(&ssif_bmc->response_buf[idx], 0, MAX_PAYLOAD_PER_TRANSACTION - idx);
+		} 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 */
+		dev_err(&ssif_bmc->client->dev,
+			"Error: Unexpected SMBus command received 0x%x\n", ssif_bmc->smbus_cmd);
+		break;
+	}
+
+	ssif_bmc->nbytes_processed += response_len;
+}
+
+/* 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 */
+			dev_err(&ssif_bmc->client->dev,
+				"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 */
+		dev_err(&ssif_bmc->client->dev,
+			"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) {
+			dev_err(&ssif_bmc->client->dev, "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) {
+			dev_err(&ssif_bmc->client->dev, "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 */
+	dev_err(&ssif_bmc->client->dev,
+		"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)) {
+		dev_err(&ssif_bmc->client->dev, "Received invalid PEC\n");
+		return;
+	}
+
+	if (cmd == SSIF_IPMI_SINGLEPART_WRITE || cmd == SSIF_IPMI_MULTIPART_WRITE_END)
+		handle_request(ssif_bmc);
+}
+
+static void initialize_transfer(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
+{
+	/* SMBUS command can vary (single or multi-part) */
+	ssif_bmc->smbus_cmd = *val;
+	ssif_bmc->msg_idx++;
+
+	if (ssif_bmc->smbus_cmd == SSIF_IPMI_SINGLEPART_WRITE ||
+	    ssif_bmc->smbus_cmd == SSIF_IPMI_MULTIPART_WRITE_START) {
+		/*
+		 * The response can be delayed in BMC causing host SSIF driver
+		 * to timeout and send a new request once BMC slave is ready.
+		 * In that case check for pending response and clear it
+		 */
+		if (ssif_bmc->response_in_progress) {
+			dev_err(&ssif_bmc->client->dev,
+				"Warn: SSIF new request with pending response");
+			complete_response(ssif_bmc);
+		}
+	} else if (unlikely(ssif_bmc->smbus_cmd != SSIF_IPMI_SINGLEPART_READ &&
+			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_WRITE_MIDDLE &&
+			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_WRITE_END &&
+			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_READ_START &&
+			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_READ_MIDDLE)) {
+		/* Unknown command, clear it */
+		dev_err(&ssif_bmc->client->dev, "Warn: Unknown SMBus command");
+		complete_response(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)
+{
+	unsigned long flags;
+	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
+
+	spin_lock_irqsave(&ssif_bmc->lock, flags);
+
+	/* 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)
+			*val = ssif_bmc->response.len;
+		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:
+		if (ssif_bmc->msg_idx)
+			handle_write_received(ssif_bmc, val);
+		else
+			initialize_transfer(ssif_bmc, val);
+
+		break;
+
+	case I2C_SLAVE_STOP:
+		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_irqrestore(&ssif_bmc->lock, flags);
+
+	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;
+
+	/* 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..eab540061f14
--- /dev/null
+++ b/drivers/char/ipmi/ssif_bmc.h
@@ -0,0 +1,93 @@
+/* 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			running;
+	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 */
+	spinlock_t		lock;
+	wait_queue_head_t	wait_queue;
+	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__ */
-- 
2.28.0

Re: [PATCH v3 2/7] ipmi: ssif_bmc: Add SSIF BMC driver

[Corey Minyard]

On Wed, May 19, 2021 at 02:49:29PM +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.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + Removed redundant license info [Joel]
>   + Switched to use traditional if-else [Joel]
>   + Removed unused ssif_bmc_ioctl() [Joel]
>   + Made handle_request()/complete_response() to return void [Joel]
>   + Refactored send_ssif_bmc_response()/receive_ssif_bmc_request() [Corey]
>   + Removed mutex [Corey]
>   + Use spin_lock/unlock_irqsave/restore in callback [Corey]
>   + Removed the unnecessary memset [Corey]
>   + Switch to use dev_err() [Corey]
> 
> v2:
>   + Fixed compiling error with COMPILE_TEST for arc
> 
>  drivers/char/ipmi/Kconfig    |  11 +
>  drivers/char/ipmi/Makefile   |   1 +
>  drivers/char/ipmi/ssif_bmc.c | 605 +++++++++++++++++++++++++++++++++++
>  drivers/char/ipmi/ssif_bmc.h |  93 ++++++
>  4 files changed, 710 insertions(+)
>  create mode 100644 drivers/char/ipmi/ssif_bmc.c
>  create mode 100644 drivers/char/ipmi/ssif_bmc.h
> 
> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> index 07847d9a459a..ad5c5161bcd6 100644
> --- a/drivers/char/ipmi/Kconfig
> +++ b/drivers/char/ipmi/Kconfig
> @@ -133,6 +133,17 @@ 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 IPMB_DEVICE_INTERFACE
>  	tristate 'IPMB Interface handler'
>  	depends on I2C
> diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
> index 0822adc2ec41..d04a214d74c4 100644
> --- a/drivers/char/ipmi/Makefile
> +++ b/drivers/char/ipmi/Makefile
> @@ -27,3 +27,4 @@ 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
> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
> new file mode 100644
> index 000000000000..d0ea7059b1db
> --- /dev/null
> +++ b/drivers/char/ipmi/ssif_bmc.c
> @@ -0,0 +1,605 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * The driver for BMC side of SSIF interface
> + *
> + * Copyright (c) 2021, Ampere Computing LLC
> + *
> + */
> +
> +#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"
> +
> +/* 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;
> +
> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	while (!ssif_bmc->request_available) {
> +		if (file->f_flags & O_NONBLOCK) {

There is no real need to check this under the lock.  It would simplify
the end of this function if you checked this outside the lock.

> +			ret = -EAGAIN;
> +			goto out_unlock;
> +		}
> +		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +		ret = wait_event_interruptible(ssif_bmc->wait_queue,
> +					       ssif_bmc->request_available);
> +		if (ret)
> +			return ret;
> +		spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	}
> +
> +	count = min_t(ssize_t, ssif_msg_len(&ssif_bmc->request),
> +		      min_t(ssize_t, sizeof(struct ssif_msg), count));

Just curious, what happens if userland supplies a buffer that is too
small?  Is data lost?  In that case, I would think returning an error
here would be the better thing, as opposed to silently truncating the
message.

> +	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);
> +	goto out;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +out:
> +	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;
> +
> +	ret = copy_from_user(&msg, buf, count);
> +	if (ret)
> +		return ret;
> +
> +	if (!msg.len || count < ssif_msg_len(&msg))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	while (ssif_bmc->response_in_progress) {
> +		if (file->f_flags & O_NONBLOCK) {

Same comment as O_NONBLOCK before.

> +			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)
> +			goto out;
> +		spin_lock_irqsave(&ssif_bmc->lock, flags);
> +	}
> +
> +	memcpy(&ssif_bmc->response, &msg, count);
> +	ssif_bmc->is_singlepart_read = (ssif_msg_len(&msg) <= MAX_PAYLOAD_PER_TRANSACTION + 1);
> +	ssif_bmc->response_in_progress = true;
> +	if (ssif_bmc->set_ssif_bmc_status)
> +		ssif_bmc->set_ssif_bmc_status(ssif_bmc, SSIF_BMC_READY);
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> +
> +out:
> +	return (ret < 0) ? ret : count;
> +}
> +
> +static int ssif_bmc_open(struct inode *inode, struct file *file)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
> +	int ret = 0;
> +
> +	spin_lock_irq(&ssif_bmc->lock);
> +	if (!ssif_bmc->running)
> +		ssif_bmc->running = 1;
> +	else
> +		ret = -EBUSY;
> +	spin_unlock_irq(&ssif_bmc->lock);
> +
> +	return ret;
> +}
> +
> +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;
> +
> +	poll_wait(file, &ssif_bmc->wait_queue, wait);
> +
> +	spin_lock_irq(&ssif_bmc->lock);
> +	/*
> +	 * The request message is now available so userspace application can
> +	 * get the request
> +	 */
> +	if (ssif_bmc->request_available)
> +		mask |= POLLIN;
> +
> +	spin_unlock_irq(&ssif_bmc->lock);
> +
> +	return mask;
> +}
> +
> +static int ssif_bmc_release(struct inode *inode, struct file *file)
> +{
> +	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
> +
> +	spin_lock_irq(&ssif_bmc->lock);
> +	ssif_bmc->running = 0;
> +	spin_unlock_irq(&ssif_bmc->lock);
> +
> +	return 0;
> +}
> +
> +/*
> + * System calls to device interface for user apps
> + */
> +static const struct file_operations ssif_bmc_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= ssif_bmc_open,
> +	.read		= ssif_bmc_read,
> +	.write		= ssif_bmc_write,
> +	.release	= ssif_bmc_release,
> +	.poll		= ssif_bmc_poll,
> +};
> +
> +/* Called with ssif_bmc->lock held. */
> +static void 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.
> +	 */
> +	wake_up_all(&ssif_bmc->wait_queue);
> +}
> +
> +/* Called with ssif_bmc->lock held. */
> +static void 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;
> +	wake_up_all(&ssif_bmc->wait_queue);
> +}
> +
> +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
> +			 *
> +			 */
> +			*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;
> +			/* Clean the buffer */
> +			memset(&ssif_bmc->response_buf[idx], 0, MAX_PAYLOAD_PER_TRANSACTION - idx);
> +		} 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 */
> +		dev_err(&ssif_bmc->client->dev,
> +			"Error: Unexpected SMBus command received 0x%x\n", ssif_bmc->smbus_cmd);
> +		break;
> +	}
> +
> +	ssif_bmc->nbytes_processed += response_len;
> +}
> +
> +/* 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 */
> +			dev_err(&ssif_bmc->client->dev,
> +				"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))

Shouldn't you abort the message if you get too much data?  Otherwise you
get silent truncation.

> +		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;

Generally you put the space after the break, not before.  You did this
right before, but didn't from here down in the file.

> +	case SSIF_IPMI_MULTIPART_WRITE_START:
> +		/* Reset length to zero */
> +		if (ssif_bmc->msg_idx == 1)
> +			ssif_bmc->request.len = 0;

What happens if you get this and you are in the middle of a message?

> +
> +		fallthrough;
> +	case SSIF_IPMI_MULTIPART_WRITE_MIDDLE:
> +	case SSIF_IPMI_MULTIPART_WRITE_END:

What happens if you get these and you are not in a message?

> +		/* 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 */
> +		dev_err(&ssif_bmc->client->dev,
> +			"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) {
> +			dev_err(&ssif_bmc->client->dev, "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) {
> +			dev_err(&ssif_bmc->client->dev, "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 */
> +	dev_err(&ssif_bmc->client->dev,
> +		"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)) {
> +		dev_err(&ssif_bmc->client->dev, "Received invalid PEC\n");
> +		return;
> +	}
> +
> +	if (cmd == SSIF_IPMI_SINGLEPART_WRITE || cmd == SSIF_IPMI_MULTIPART_WRITE_END)
> +		handle_request(ssif_bmc);
> +}
> +
> +static void initialize_transfer(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
> +{
> +	/* SMBUS command can vary (single or multi-part) */
> +	ssif_bmc->smbus_cmd = *val;
> +	ssif_bmc->msg_idx++;
> +
> +	if (ssif_bmc->smbus_cmd == SSIF_IPMI_SINGLEPART_WRITE ||
> +	    ssif_bmc->smbus_cmd == SSIF_IPMI_MULTIPART_WRITE_START) {
> +		/*
> +		 * The response can be delayed in BMC causing host SSIF driver
> +		 * to timeout and send a new request once BMC slave is ready.
> +		 * In that case check for pending response and clear it
> +		 */
> +		if (ssif_bmc->response_in_progress) {
> +			dev_err(&ssif_bmc->client->dev,
> +				"Warn: SSIF new request with pending response");
> +			complete_response(ssif_bmc);
> +		}
> +	} else if (unlikely(ssif_bmc->smbus_cmd != SSIF_IPMI_SINGLEPART_READ &&
> +			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_WRITE_MIDDLE &&
> +			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_WRITE_END &&
> +			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_READ_START &&
> +			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_READ_MIDDLE)) {
> +		/* Unknown command, clear it */
> +		dev_err(&ssif_bmc->client->dev, "Warn: Unknown SMBus command");
> +		complete_response(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)
> +{
> +	unsigned long flags;
> +	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
> +
> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
> +
> +	/* 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)
> +			*val = ssif_bmc->response.len;
> +		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:
> +		if (ssif_bmc->msg_idx)
> +			handle_write_received(ssif_bmc, val);
> +		else
> +			initialize_transfer(ssif_bmc, val);
> +
> +		break;
> +
> +	case I2C_SLAVE_STOP:
> +		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_irqrestore(&ssif_bmc->lock, flags);
> +
> +	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;
> +
> +	/* 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..eab540061f14
> --- /dev/null
> +++ b/drivers/char/ipmi/ssif_bmc.h
> @@ -0,0 +1,93 @@
> +/* 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			running;
> +	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 */
> +	spinlock_t		lock;
> +	wait_queue_head_t	wait_queue;
> +	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__ */
> -- 
> 2.28.0
> 

Re: [PATCH v3 2/7] ipmi: ssif_bmc: Add SSIF BMC driver

[Quan Nguyen]

On 19/05/2021 19:30, Corey Minyard wrote:
> On Wed, May 19, 2021 at 02:49:29PM +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.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + Removed redundant license info [Joel]
>>    + Switched to use traditional if-else [Joel]
>>    + Removed unused ssif_bmc_ioctl() [Joel]
>>    + Made handle_request()/complete_response() to return void [Joel]
>>    + Refactored send_ssif_bmc_response()/receive_ssif_bmc_request() [Corey]
>>    + Removed mutex [Corey]
>>    + Use spin_lock/unlock_irqsave/restore in callback [Corey]
>>    + Removed the unnecessary memset [Corey]
>>    + Switch to use dev_err() [Corey]
>>
>> v2:
>>    + Fixed compiling error with COMPILE_TEST for arc
>>
>>   drivers/char/ipmi/Kconfig    |  11 +
>>   drivers/char/ipmi/Makefile   |   1 +
>>   drivers/char/ipmi/ssif_bmc.c | 605 +++++++++++++++++++++++++++++++++++
>>   drivers/char/ipmi/ssif_bmc.h |  93 ++++++
>>   4 files changed, 710 insertions(+)
>>   create mode 100644 drivers/char/ipmi/ssif_bmc.c
>>   create mode 100644 drivers/char/ipmi/ssif_bmc.h
>>
>> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
>> index 07847d9a459a..ad5c5161bcd6 100644
>> --- a/drivers/char/ipmi/Kconfig
>> +++ b/drivers/char/ipmi/Kconfig
>> @@ -133,6 +133,17 @@ 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 IPMB_DEVICE_INTERFACE
>>   	tristate 'IPMB Interface handler'
>>   	depends on I2C
>> diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
>> index 0822adc2ec41..d04a214d74c4 100644
>> --- a/drivers/char/ipmi/Makefile
>> +++ b/drivers/char/ipmi/Makefile
>> @@ -27,3 +27,4 @@ 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
>> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
>> new file mode 100644
>> index 000000000000..d0ea7059b1db
>> --- /dev/null
>> +++ b/drivers/char/ipmi/ssif_bmc.c
>> @@ -0,0 +1,605 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * The driver for BMC side of SSIF interface
>> + *
>> + * Copyright (c) 2021, Ampere Computing LLC
>> + *
>> + */
>> +
>> +#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"
>> +
>> +/* 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;
>> +
>> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
>> +	while (!ssif_bmc->request_available) {
>> +		if (file->f_flags & O_NONBLOCK) {
> 
> There is no real need to check this under the lock.  It would simplify
> the end of this function if you checked this outside the lock.
> 
Thanks for your reviewing.

Will apply change like below in next version:

         spin_lock_irqsave(&ssif_bmc->lock, flags);
         while (!ssif_bmc->request_available) {
-               if (file->f_flags & O_NONBLOCK) {
-                       ret = -EAGAIN;
-                       goto out_unlock;
-               }
                 spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+               if (file->f_flags & O_NONBLOCK)
+                       return -EAGAIN;
                 ret = wait_event_interruptible(ssif_bmc->wait_queue,

ssif_bmc->request_available);
                 if (ret)

>> +			ret = -EAGAIN;
>> +			goto out_unlock;
>> +		}
>> +		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
>> +		ret = wait_event_interruptible(ssif_bmc->wait_queue,
>> +					       ssif_bmc->request_available);
>> +		if (ret)
>> +			return ret;
>> +		spin_lock_irqsave(&ssif_bmc->lock, flags);
>> +	}
>> +
>> +	count = min_t(ssize_t, ssif_msg_len(&ssif_bmc->request),
>> +		      min_t(ssize_t, sizeof(struct ssif_msg), count));
> 
> Just curious, what happens if userland supplies a buffer that is too
> small?  Is data lost?  In that case, I would think returning an error
> here would be the better thing, as opposed to silently truncating the
> message.
> 
Will return -EINVAL if buffer is too small in next version

>> +	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);
>> +	goto out;
>> +
>> +out_unlock:
>> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
>> +out:
>> +	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;
>> +
>> +	ret = copy_from_user(&msg, buf, count);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!msg.len || count < ssif_msg_len(&msg))
>> +		return -EINVAL;
>> +
>> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
>> +	while (ssif_bmc->response_in_progress) {
>> +		if (file->f_flags & O_NONBLOCK) {
> 
> Same comment as O_NONBLOCK before.
>
Will apply this follow the above in ssif_bmc_read()

>> +			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)
>> +			goto out;
>> +		spin_lock_irqsave(&ssif_bmc->lock, flags);
>> +	}
>> +
>> +	memcpy(&ssif_bmc->response, &msg, count);
>> +	ssif_bmc->is_singlepart_read = (ssif_msg_len(&msg) <= MAX_PAYLOAD_PER_TRANSACTION + 1);
>> +	ssif_bmc->response_in_progress = true;
>> +	if (ssif_bmc->set_ssif_bmc_status)
>> +		ssif_bmc->set_ssif_bmc_status(ssif_bmc, SSIF_BMC_READY);
>> +
>> +out_unlock:
>> +	spin_unlock_irqrestore(&ssif_bmc->lock, flags);
>> +
>> +out:
>> +	return (ret < 0) ? ret : count;
>> +}
>> +
>> +static int ssif_bmc_open(struct inode *inode, struct file *file)
>> +{
>> +	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
>> +	int ret = 0;
>> +
>> +	spin_lock_irq(&ssif_bmc->lock);
>> +	if (!ssif_bmc->running)
>> +		ssif_bmc->running = 1;
>> +	else
>> +		ret = -EBUSY;
>> +	spin_unlock_irq(&ssif_bmc->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +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;
>> +
>> +	poll_wait(file, &ssif_bmc->wait_queue, wait);
>> +
>> +	spin_lock_irq(&ssif_bmc->lock);
>> +	/*
>> +	 * The request message is now available so userspace application can
>> +	 * get the request
>> +	 */
>> +	if (ssif_bmc->request_available)
>> +		mask |= POLLIN;
>> +
>> +	spin_unlock_irq(&ssif_bmc->lock);
>> +
>> +	return mask;
>> +}
>> +
>> +static int ssif_bmc_release(struct inode *inode, struct file *file)
>> +{
>> +	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
>> +
>> +	spin_lock_irq(&ssif_bmc->lock);
>> +	ssif_bmc->running = 0;
>> +	spin_unlock_irq(&ssif_bmc->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * System calls to device interface for user apps
>> + */
>> +static const struct file_operations ssif_bmc_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.open		= ssif_bmc_open,
>> +	.read		= ssif_bmc_read,
>> +	.write		= ssif_bmc_write,
>> +	.release	= ssif_bmc_release,
>> +	.poll		= ssif_bmc_poll,
>> +};
>> +
>> +/* Called with ssif_bmc->lock held. */
>> +static void 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.
>> +	 */
>> +	wake_up_all(&ssif_bmc->wait_queue);
>> +}
>> +
>> +/* Called with ssif_bmc->lock held. */
>> +static void 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;
>> +	wake_up_all(&ssif_bmc->wait_queue);
>> +}
>> +
>> +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
>> +			 *
>> +			 */
>> +			*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;
>> +			/* Clean the buffer */
>> +			memset(&ssif_bmc->response_buf[idx], 0, MAX_PAYLOAD_PER_TRANSACTION - idx);
>> +		} 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 */
>> +		dev_err(&ssif_bmc->client->dev,
>> +			"Error: Unexpected SMBus command received 0x%x\n", ssif_bmc->smbus_cmd);
>> +		break;
>> +	}
>> +
>> +	ssif_bmc->nbytes_processed += response_len;
>> +}
>> +
>> +/* 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 */
>> +			dev_err(&ssif_bmc->client->dev,
>> +				"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))
> 
> Shouldn't you abort the message if you get too much data?  Otherwise you
> get silent truncation.
> 
Will try to abort bad request in next version.

>> +		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;
> 
> Generally you put the space after the break, not before.  You did this
> right before, but didn't from here down in the file.
> 
Will fix them in next version.

>> +	case SSIF_IPMI_MULTIPART_WRITE_START:
>> +		/* Reset length to zero */
>> +		if (ssif_bmc->msg_idx == 1)
>> +			ssif_bmc->request.len = 0;
> 
> What happens if you get this and you are in the middle of a message?
> 
As per specs, this message should be discarded. This part is still 
missing. I'll try to add this feature in next version.

>> +
>> +		fallthrough;
>> +	case SSIF_IPMI_MULTIPART_WRITE_MIDDLE:
>> +	case SSIF_IPMI_MULTIPART_WRITE_END:
> 
> What happens if you get these and you are not in a message?
> 
Same as my comment above.

>> +		/* 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 */
>> +		dev_err(&ssif_bmc->client->dev,
>> +			"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) {
>> +			dev_err(&ssif_bmc->client->dev, "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) {
>> +			dev_err(&ssif_bmc->client->dev, "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 */
>> +	dev_err(&ssif_bmc->client->dev,
>> +		"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)) {
>> +		dev_err(&ssif_bmc->client->dev, "Received invalid PEC\n");
>> +		return;
>> +	}
>> +
>> +	if (cmd == SSIF_IPMI_SINGLEPART_WRITE || cmd == SSIF_IPMI_MULTIPART_WRITE_END)
>> +		handle_request(ssif_bmc);
>> +}
>> +
>> +static void initialize_transfer(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
>> +{
>> +	/* SMBUS command can vary (single or multi-part) */
>> +	ssif_bmc->smbus_cmd = *val;
>> +	ssif_bmc->msg_idx++;
>> +
>> +	if (ssif_bmc->smbus_cmd == SSIF_IPMI_SINGLEPART_WRITE ||
>> +	    ssif_bmc->smbus_cmd == SSIF_IPMI_MULTIPART_WRITE_START) {
>> +		/*
>> +		 * The response can be delayed in BMC causing host SSIF driver
>> +		 * to timeout and send a new request once BMC slave is ready.
>> +		 * In that case check for pending response and clear it
>> +		 */
>> +		if (ssif_bmc->response_in_progress) {
>> +			dev_err(&ssif_bmc->client->dev,
>> +				"Warn: SSIF new request with pending response");
>> +			complete_response(ssif_bmc);
>> +		}
>> +	} else if (unlikely(ssif_bmc->smbus_cmd != SSIF_IPMI_SINGLEPART_READ &&
>> +			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_WRITE_MIDDLE &&
>> +			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_WRITE_END &&
>> +			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_READ_START &&
>> +			    ssif_bmc->smbus_cmd != SSIF_IPMI_MULTIPART_READ_MIDDLE)) {
>> +		/* Unknown command, clear it */
>> +		dev_err(&ssif_bmc->client->dev, "Warn: Unknown SMBus command");
>> +		complete_response(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)
>> +{
>> +	unsigned long flags;
>> +	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
>> +
>> +	spin_lock_irqsave(&ssif_bmc->lock, flags);
>> +
>> +	/* 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)
>> +			*val = ssif_bmc->response.len;
>> +		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:
>> +		if (ssif_bmc->msg_idx)
>> +			handle_write_received(ssif_bmc, val);
>> +		else
>> +			initialize_transfer(ssif_bmc, val);
>> +
>> +		break;
>> +
>> +	case I2C_SLAVE_STOP:
>> +		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_irqrestore(&ssif_bmc->lock, flags);
>> +
>> +	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;
>> +
>> +	/* 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..eab540061f14
>> --- /dev/null
>> +++ b/drivers/char/ipmi/ssif_bmc.h
>> @@ -0,0 +1,93 @@
>> +/* 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			running;
>> +	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 */
>> +	spinlock_t		lock;
>> +	wait_queue_head_t	wait_queue;
>> +	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__ */
>> -- 
>> 2.28.0
>>

[PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK

[Quan Nguyen]

It is observed that in normal condition, when the last byte sent by
slave, the Tx Done with NAK irq will raise.
But it is also observed that sometimes master issues next transaction
too quick while the slave irq handler is not yet invoked and Tx Done
with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
This Tx Done with NAK irq is raised together with the Slave Match and
Rx Done irq of the next coming transaction from master.
Unfortunately, the current slave irq handler handles the Slave Match and
Rx Done only in higher priority and ignore the Tx Done with NAK, causing
the complain as below:
"aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
0x00000086, but was 0x00000084"

This commit handles this case by emitting a Slave Stop event for the
Tx Done with NAK before processing Slave Match and Rx Done for the
coming transaction from master.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
v3:
  + First introduce in v3 [Quan]

 drivers/i2c/busses/i2c-aspeed.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 724bf30600d6..3fb37c3f23d4 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -254,6 +254,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 
 	/* Slave was requested, restart state machine. */
 	if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
+		if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
+		    bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
+			irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
+			i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+		}
 		irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
 		bus->slave_state = ASPEED_I2C_SLAVE_START;
 	}
-- 
2.28.0

Re: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK

[Joel Stanley]

Ryan, can you please review this change?

On Wed, 19 May 2021 at 07:50, Quan Nguyen <quan@os.amperecomputing.com> wrote:
>
> It is observed that in normal condition, when the last byte sent by
> slave, the Tx Done with NAK irq will raise.
> But it is also observed that sometimes master issues next transaction
> too quick while the slave irq handler is not yet invoked and Tx Done
> with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
> This Tx Done with NAK irq is raised together with the Slave Match and
> Rx Done irq of the next coming transaction from master.
> Unfortunately, the current slave irq handler handles the Slave Match and
> Rx Done only in higher priority and ignore the Tx Done with NAK, causing
> the complain as below:
> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
> 0x00000086, but was 0x00000084"
>
> This commit handles this case by emitting a Slave Stop event for the
> Tx Done with NAK before processing Slave Match and Rx Done for the
> coming transaction from master.

It sounds like this patch is independent of the rest of the series,
and can go in on it's own. Please send it separately to the i2c
maintainers and add a suitable Fixes line, such as:

  Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")

>
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + First introduce in v3 [Quan]
>
>  drivers/i2c/busses/i2c-aspeed.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 724bf30600d6..3fb37c3f23d4 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -254,6 +254,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>
>         /* Slave was requested, restart state machine. */
>         if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {

Can you explain why you need to do this handing inside the SLAVE_MATCH case?

Could you instead move the TX_NAK handling to be above the SLAVE_MATCH case?

> +               if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
> +                   bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {

Either way, this needs a comment to explain what we're working around.

> +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
> +                       i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
> +               }
>                 irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>                 bus->slave_state = ASPEED_I2C_SLAVE_START;
>         }
> --
> 2.28.0
>

RE: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK

[Ryan Chen]

> -----Original Message-----
> From: Joel Stanley <joel@jms.id.au>
> Sent: Thursday, May 20, 2021 7:29 AM
> To: Quan Nguyen <quan@os.amperecomputing.com>; Ryan Chen
> <ryan_chen@aspeedtech.com>
> Cc: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;
> Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
> <brendanhiggins@google.com>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
> devicetree <devicetree@vger.kernel.org>; Linux ARM
> <linux-arm-kernel@lists.infradead.org>; linux-aspeed
> <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; linux-i2c@vger.kernel.org; Open Source
> Submission <patches@amperecomputing.com>; Phong Vo
> <phong@os.amperecomputing.com>; Thang Q . Nguyen
> <thang@os.amperecomputing.com>; OpenBMC Maillist
> <openbmc@lists.ozlabs.org>
> Subject: Re: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK
> 
> Ryan, can you please review this change?
> 
> On Wed, 19 May 2021 at 07:50, Quan Nguyen
> <quan@os.amperecomputing.com> wrote:
> >
> > It is observed that in normal condition, when the last byte sent by
> > slave, the Tx Done with NAK irq will raise.
> > But it is also observed that sometimes master issues next transaction
> > too quick while the slave irq handler is not yet invoked and Tx Done
> > with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
> > This Tx Done with NAK irq is raised together with the Slave Match and
> > Rx Done irq of the next coming transaction from master.
> > Unfortunately, the current slave irq handler handles the Slave Match
> > and Rx Done only in higher priority and ignore the Tx Done with NAK,
> > causing the complain as below:
> > "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
> > 0x00000086, but was 0x00000084"
> >
> > This commit handles this case by emitting a Slave Stop event for the
> > Tx Done with NAK before processing Slave Match and Rx Done for the
> > coming transaction from master.
> 
> It sounds like this patch is independent of the rest of the series, and can go in
> on it's own. Please send it separately to the i2c maintainers and add a suitable
> Fixes line, such as:
> 
>   Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C
> driver")
> 
> >
> > Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> > ---
> > v3:
> >   + First introduce in v3 [Quan]
> >
> >  drivers/i2c/busses/i2c-aspeed.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-aspeed.c
> > b/drivers/i2c/busses/i2c-aspeed.c index 724bf30600d6..3fb37c3f23d4
> > 100644
> > --- a/drivers/i2c/busses/i2c-aspeed.c
> > +++ b/drivers/i2c/busses/i2c-aspeed.c
> > @@ -254,6 +254,11 @@ static u32 aspeed_i2c_slave_irq(struct
> > aspeed_i2c_bus *bus, u32 irq_status)
> >
> >         /* Slave was requested, restart state machine. */
> >         if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> 
> Can you explain why you need to do this handing inside the SLAVE_MATCH
> case?
> 
> Could you instead move the TX_NAK handling to be above the SLAVE_MATCH
> case?
> 
> > +               if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
> > +                   bus->slave_state ==
> > + ASPEED_I2C_SLAVE_READ_PROCESSED) {
> 
> Either way, this needs a comment to explain what we're working around.
> 
> > +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
> > +                       i2c_slave_event(slave, I2C_SLAVE_STOP,
> &value);

According the patch assume slave receive TX_NAK will be go to SLAVE_STOP state?

> > +               }
> >                 irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> >                 bus->slave_state = ASPEED_I2C_SLAVE_START;
> >         }
> > --
> > 2.28.0
> >

Re: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK

[Quan Nguyen]

On 20/05/2021 18:28, Ryan Chen wrote:
>> -----Original Message-----
>> From: Joel Stanley <joel@jms.id.au>
>> Sent: Thursday, May 20, 2021 7:29 AM
>> To: Quan Nguyen <quan@os.amperecomputing.com>; Ryan Chen
>> <ryan_chen@aspeedtech.com>
>> Cc: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;
>> Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
>> <brendanhiggins@google.com>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
>> devicetree <devicetree@vger.kernel.org>; Linux ARM
>> <linux-arm-kernel@lists.infradead.org>; linux-aspeed
>> <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List
>> <linux-kernel@vger.kernel.org>; linux-i2c@vger.kernel.org; Open Source
>> Submission <patches@amperecomputing.com>; Phong Vo
>> <phong@os.amperecomputing.com>; Thang Q . Nguyen
>> <thang@os.amperecomputing.com>; OpenBMC Maillist
>> <openbmc@lists.ozlabs.org>
>> Subject: Re: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK
>>
>> Ryan, can you please review this change?
>>
>> On Wed, 19 May 2021 at 07:50, Quan Nguyen
>> <quan@os.amperecomputing.com> wrote:
>>>
>>> It is observed that in normal condition, when the last byte sent by
>>> slave, the Tx Done with NAK irq will raise.
>>> But it is also observed that sometimes master issues next transaction
>>> too quick while the slave irq handler is not yet invoked and Tx Done
>>> with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
>>> This Tx Done with NAK irq is raised together with the Slave Match and
>>> Rx Done irq of the next coming transaction from master.
>>> Unfortunately, the current slave irq handler handles the Slave Match
>>> and Rx Done only in higher priority and ignore the Tx Done with NAK,
>>> causing the complain as below:
>>> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
>>> 0x00000086, but was 0x00000084"
>>>
>>> This commit handles this case by emitting a Slave Stop event for the
>>> Tx Done with NAK before processing Slave Match and Rx Done for the
>>> coming transaction from master.
>>
>> It sounds like this patch is independent of the rest of the series, and can go in
>> on it's own. Please send it separately to the i2c maintainers and add a suitable
>> Fixes line, such as:
>>
>>    Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C
>> driver")
>>
>>>
>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>>> ---
>>> v3:
>>>    + First introduce in v3 [Quan]
>>>
>>>   drivers/i2c/busses/i2c-aspeed.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>>> b/drivers/i2c/busses/i2c-aspeed.c index 724bf30600d6..3fb37c3f23d4
>>> 100644
>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>> @@ -254,6 +254,11 @@ static u32 aspeed_i2c_slave_irq(struct
>>> aspeed_i2c_bus *bus, u32 irq_status)
>>>
>>>          /* Slave was requested, restart state machine. */
>>>          if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
>>
>> Can you explain why you need to do this handing inside the SLAVE_MATCH
>> case?
>>
>> Could you instead move the TX_NAK handling to be above the SLAVE_MATCH
>> case?
>>
>>> +               if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
>>> +                   bus->slave_state ==
>>> + ASPEED_I2C_SLAVE_READ_PROCESSED) {
>>
>> Either way, this needs a comment to explain what we're working around.
>>
>>> +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>>> +                       i2c_slave_event(slave, I2C_SLAVE_STOP,
>> &value);
> 
> According the patch assume slave receive TX_NAK will be go to SLAVE_STOP state?
> 
Hi Ryan,

As per my explain in other email, we need to emit one SLAVE_STOP event 
to complete the previous transaction before start processing for the 
next transaction.

- Quan

>>> +               }
>>>                  irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>>>                  bus->slave_state = ASPEED_I2C_SLAVE_START;
>>>          }
>>> --
>>> 2.28.0
>>>

Re: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK

[Quan Nguyen]

On 20/05/2021 06:28, Joel Stanley wrote:
> Ryan, can you please review this change?
> 
> On Wed, 19 May 2021 at 07:50, Quan Nguyen <quan@os.amperecomputing.com> wrote:
>>
>> It is observed that in normal condition, when the last byte sent by
>> slave, the Tx Done with NAK irq will raise.
>> But it is also observed that sometimes master issues next transaction
>> too quick while the slave irq handler is not yet invoked and Tx Done
>> with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
>> This Tx Done with NAK irq is raised together with the Slave Match and
>> Rx Done irq of the next coming transaction from master.
>> Unfortunately, the current slave irq handler handles the Slave Match and
>> Rx Done only in higher priority and ignore the Tx Done with NAK, causing
>> the complain as below:
>> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
>> 0x00000086, but was 0x00000084"
>>
>> This commit handles this case by emitting a Slave Stop event for the
>> Tx Done with NAK before processing Slave Match and Rx Done for the
>> coming transaction from master.
> 
> It sounds like this patch is independent of the rest of the series,
> and can go in on it's own. Please send it separately to the i2c
> maintainers and add a suitable Fixes line, such as:
> 
>    Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
> 
Will separate this patch into independent series in next version.

>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + First introduce in v3 [Quan]
>>
>>   drivers/i2c/busses/i2c-aspeed.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 724bf30600d6..3fb37c3f23d4 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -254,6 +254,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>
>>          /* Slave was requested, restart state machine. */
>>          if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> 
> Can you explain why you need to do this handing inside the SLAVE_MATCH case?

> Could you instead move the TX_NAK handling to be above the SLAVE_MATCH case?
>
>> +               if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
>> +                   bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
> 
> Either way, this needs a comment to explain what we're working around.
>
Let me explain with the two examples below in normal case and the case 
where this patch is for:

In normal case:
The first transaction is Slave send (Master read):
    20(addr) 03(singlepart read) 03 1c 2e d5

Then the second Master write follow as below:
    20(addr) 02(singlepart write) 02 18 08 59

The irq will raise in sequence below:

  irq      data  from-state      to-state
00000084  20    INACTIVE        WRITE_RECEIVED
00000004  03    WRITE_RECEIVED  WRITE_RECEIVED <= RX_DONE
00000084  03    WRITE_RECEIVED  READ_PROCESSED
00000001  1c    READ_PROCESSED  READ_PROCESSED <= TX_ACK
00000001  2e    READ_PROCESSED  READ_PROCESSED
00000001  d5    READ_PROCESSED  READ_PROCESSED
00000002  xx    READ_PROCESSED  INACTIVE       <= TX_NAK

00000084  20    INACTIVE        WRITE_RECEIVED <= SLAVE_MATCH & RX_DONE
00000004  02    WRITE_RECEIVED  WRITE_RECEIVED
00000084  02    WRITE_RECEIVED  WRITE_RECEIVED
00000004  18    WRITE_RECEIVED  WRITE_RECEIVED
00000004  08    WRITE_RECEIVED  WRITE_RECEIVED
00000004  59    WRITE_RECEIVED  WRITE_RECEIVED
00000010  xx    WRITE_RECEIVED  INACTIVE

But sometimes:
The first transaction is Slave send (Master read):
    20(addr) 03(singlepart read) 03 1c 42 cc a5

Then the second Master write follow as below:
    20(addr) 02(singlepart write) 03 18 42 0c 63

The irq will raise in sequence below:

  irq      data  from-state      to-state
00000084  20    INACTIVE        WRITE_RECEIVED
00000004  03    WRITE_RECEIVED  WRITE_RECEIVED
00000084  03    WRITE_RECEIVED  READ_PROCESSED
00000001  1c    READ_PROCESSED  READ_PROCESSED
00000001  42    READ_PROCESSED  READ_PROCESSED
00000001  0c    READ_PROCESSED  READ_PROCESSED
00000001  63    READ_PROCESSED  READ_PROCESSED

00000086  20    READ_PROCESSED  WRITE_RECEIVED <= both 3 irqs raised
00000004  02    WRITE_RECEIVED  WRITE_RECEIVED
00000084  03    WRITE_RECEIVED  WRITE_RECEIVED
00000004  18    WRITE_RECEIVED  WRITE_RECEIVED
00000004  42    WRITE_RECEIVED  WRITE_RECEIVED
00000004  0c    WRITE_RECEIVED  WRITE_RECEIVED
00000004  63    WRITE_RECEIVED  WRITE_RECEIVED
00000010  xx    WRITE_RECEIVED  INACTIVE

This patch is to address this case where TX_NAK, SLAVE_MATCH and RX_DONE 
are raised together.

>> +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>> +                       i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
>> +               }
>>                  irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>>                  bus->slave_state = ASPEED_I2C_SLAVE_START;
>>          }
>> --
>> 2.28.0
>>

[PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late

[Quan Nguyen]

With Tx done w/wo ACK are ack'ed early at beginning of irq handler,
it is observed that, usually, the Tx done with Ack irq raises in the
READ REQUESTED state. This is unexpected and complaint as below appear:
"Unexpected Ack on read request"

Assumed that Tx done should only be ack'ed once it was truly processed,
switch to late ack'ed this two irqs and seen this issue go away through
test with AST2500..

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
v3:
  + First introduce in v3 [Quan]

 drivers/i2c/busses/i2c-aspeed.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 3fb37c3f23d4..b2e9c8f0ddf7 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -606,8 +606,12 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
 
 	spin_lock(&bus->lock);
 	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
-	/* Ack all interrupts except for Rx done */
-	writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
+	/*
+	 * Ack all interrupts except for Rx done and
+	 * Tx done with/without ACK
+	 */
+	writel(irq_received &
+	       ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK),
 	       bus->base + ASPEED_I2C_INTR_STS_REG);
 	readl(bus->base + ASPEED_I2C_INTR_STS_REG);
 	irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
@@ -652,12 +656,18 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
 			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
 			irq_received, irq_handled);
 
-	/* Ack Rx done */
-	if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
-		writel(ASPEED_I2CD_INTR_RX_DONE,
-		       bus->base + ASPEED_I2C_INTR_STS_REG);
-		readl(bus->base + ASPEED_I2C_INTR_STS_REG);
-	}
+	/* Ack Rx done and Tx done with/without ACK */
+	/* Note: Re-use irq_handled variable */
+	irq_handled = 0;
+	if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
+		irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
+	if (irq_received & ASPEED_I2CD_INTR_TX_ACK)
+		irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
+	if (irq_received & ASPEED_I2CD_INTR_TX_NAK)
+		irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
+	writel(irq_handled, bus->base + ASPEED_I2C_INTR_STS_REG);
+	readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+
 	spin_unlock(&bus->lock);
 	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
 }
-- 
2.28.0

Re: [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late

[Joel Stanley]

On Wed, 19 May 2021 at 07:50, Quan Nguyen <quan@os.amperecomputing.com> wrote:
>
> With Tx done w/wo ACK are ack'ed early at beginning of irq handler,

Is w/wo a typo? If not, please write the full words ("with and without")

> it is observed that, usually, the Tx done with Ack irq raises in the
> READ REQUESTED state. This is unexpected and complaint as below appear:
> "Unexpected Ack on read request"
>
> Assumed that Tx done should only be ack'ed once it was truly processed,
> switch to late ack'ed this two irqs and seen this issue go away through
> test with AST2500..

Please read Guneter's commit message
2be6b47211e17e6c90ead40d24d2a5cc815f2d5c to confirm that your changes
do not invalidate the fix that they made.  Add them to CC for review.

Again, this is a fix that is independent of the ssif work. Please send
it separately with a Fixes line.

>
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + First introduce in v3 [Quan]
>
>  drivers/i2c/busses/i2c-aspeed.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 3fb37c3f23d4..b2e9c8f0ddf7 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -606,8 +606,12 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>
>         spin_lock(&bus->lock);
>         irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> -       /* Ack all interrupts except for Rx done */
> -       writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> +       /*
> +        * Ack all interrupts except for Rx done and
> +        * Tx done with/without ACK

Nit: this comment can be on one line.


> +        */
> +       writel(irq_received &
> +              ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK),
>                bus->base + ASPEED_I2C_INTR_STS_REG);
>         readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>         irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
> @@ -652,12 +656,18 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>                         "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>                         irq_received, irq_handled);
>
> -       /* Ack Rx done */
> -       if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
> -               writel(ASPEED_I2CD_INTR_RX_DONE,
> -                      bus->base + ASPEED_I2C_INTR_STS_REG);
> -               readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> -       }
> +       /* Ack Rx done and Tx done with/without ACK */
> +       /* Note: Re-use irq_handled variable */

I'm not sure what this note means.

> +       irq_handled = 0;
> +       if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> +               irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
> +       if (irq_received & ASPEED_I2CD_INTR_TX_ACK)
> +               irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
> +       if (irq_received & ASPEED_I2CD_INTR_TX_NAK)
> +               irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
> +       writel(irq_handled, bus->base + ASPEED_I2C_INTR_STS_REG);

Are you intentionally only acking the bits that are set when we read
from STS_REG at the start of the handler? If not, we could write this
instead:

writel(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK |
ASPEED_I2CD_INTR_TX_NAK,
        bus->base + ASPEED_I2C_INTR_STS_REG);

If you only want to ack the bits that are set, then do this:

  writel(irq_received &
            (ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK |
ASPEED_I2CD_INTR_TX_NAK),
         bus->base + ASPEED_I2C_INTR_STS_REG);

That way, you can avoid all of the tests.

> +       readl(bus->base + ASPEED_I2C_INTR_STS_REG);

When you move this, please add a comment that reminds us why we do a
write-then-read (see commit c926c87b8e36dcc0ea5c2a0a0227ed4f32d0516a).

> +
>         spin_unlock(&bus->lock);
>         return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>  }
> --
> 2.28.0
>

Re: [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late

[Guenter Roeck]

On 5/19/21 4:43 PM, Joel Stanley wrote:
> On Wed, 19 May 2021 at 07:50, Quan Nguyen <quan@os.amperecomputing.com> wrote:
>>
>> With Tx done w/wo ACK are ack'ed early at beginning of irq handler,
> 
> Is w/wo a typo? If not, please write the full words ("with and without")
> 
>> it is observed that, usually, the Tx done with Ack irq raises in the
>> READ REQUESTED state. This is unexpected and complaint as below appear:
>> "Unexpected Ack on read request"
>>
>> Assumed that Tx done should only be ack'ed once it was truly processed,
>> switch to late ack'ed this two irqs and seen this issue go away through
>> test with AST2500..
> 
> Please read Guneter's commit message
> 2be6b47211e17e6c90ead40d24d2a5cc815f2d5c to confirm that your changes
> do not invalidate the fix that they made.  Add them to CC for review.
>  

This might re-introduce a race condition if the code that is handling
Tx done sends another byte without acknowledging the original interrupt,
and another Tx done (or Tx nack) interrupt is received before the interrupt
handler returns. If that happens, the second Tx done interrupt would be
acknowledged but not be handled, and transmit would stall. That may well be
what I had observed at the time but it is too long ago to remember, sorry.

> Again, this is a fix that is independent of the ssif work. Please send
> it separately with a Fixes line.
> 
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + First introduce in v3 [Quan]
>>
>>   drivers/i2c/busses/i2c-aspeed.c | 26 ++++++++++++++++++--------
>>   1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 3fb37c3f23d4..b2e9c8f0ddf7 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -606,8 +606,12 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>
>>          spin_lock(&bus->lock);
>>          irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> -       /* Ack all interrupts except for Rx done */
>> -       writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
>> +       /*
>> +        * Ack all interrupts except for Rx done and
>> +        * Tx done with/without ACK
> 
> Nit: this comment can be on one line.
> 
> 
>> +        */
>> +       writel(irq_received &
>> +              ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK),
>>                 bus->base + ASPEED_I2C_INTR_STS_REG);
>>          readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>          irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
>> @@ -652,12 +656,18 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>                          "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>                          irq_received, irq_handled);
>>
>> -       /* Ack Rx done */
>> -       if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
>> -               writel(ASPEED_I2CD_INTR_RX_DONE,
>> -                      bus->base + ASPEED_I2C_INTR_STS_REG);
>> -               readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> -       }
>> +       /* Ack Rx done and Tx done with/without ACK */
>> +       /* Note: Re-use irq_handled variable */
> 
> I'm not sure what this note means.
> 
>> +       irq_handled = 0;
>> +       if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
>> +               irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
>> +       if (irq_received & ASPEED_I2CD_INTR_TX_ACK)
>> +               irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>> +       if (irq_received & ASPEED_I2CD_INTR_TX_NAK)
>> +               irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>> +       writel(irq_handled, bus->base + ASPEED_I2C_INTR_STS_REG);
> 
> Are you intentionally only acking the bits that are set when we read
> from STS_REG at the start of the handler? If not, we could write this
> instead:
> 
> writel(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK |
> ASPEED_I2CD_INTR_TX_NAK,
>          bus->base + ASPEED_I2C_INTR_STS_REG);
> 

This would clear those bits unconditionally even if they were not handled.

> If you only want to ack the bits that are set, then do this:
> 
>    writel(irq_received &
>              (ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK |
> ASPEED_I2CD_INTR_TX_NAK),
>           bus->base + ASPEED_I2C_INTR_STS_REG);
> 
> That way, you can avoid all of the tests.
> 
Or
	irq_handled = irq_received &
		(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK);
	writel(irq_handled, bus->base + ASPEED_I2C_INTR_STS_REG);

if the idea was to avoid the long statement.

Guenter

>> +       readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> 
> When you move this, please add a comment that reminds us why we do a
> write-then-read (see commit c926c87b8e36dcc0ea5c2a0a0227ed4f32d0516a).
> 
>> +
>>          spin_unlock(&bus->lock);
>>          return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>   }
>> --
>> 2.28.0
>>

Re: [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late

[Quan Nguyen]

On 20/05/2021 08:19, Guenter Roeck wrote:
> On 5/19/21 4:43 PM, Joel Stanley wrote:
>> On Wed, 19 May 2021 at 07:50, Quan Nguyen 
>> <quan@os.amperecomputing.com> wrote:
>>>
>>> With Tx done w/wo ACK are ack'ed early at beginning of irq handler,
>>
>> Is w/wo a typo? If not, please write the full words ("with and without")
>>
>>> it is observed that, usually, the Tx done with Ack irq raises in the
>>> READ REQUESTED state. This is unexpected and complaint as below appear:
>>> "Unexpected Ack on read request"
>>>
>>> Assumed that Tx done should only be ack'ed once it was truly processed,
>>> switch to late ack'ed this two irqs and seen this issue go away through
>>> test with AST2500..
>>
>> Please read Guneter's commit message
>> 2be6b47211e17e6c90ead40d24d2a5cc815f2d5c to confirm that your changes
>> do not invalidate the fix that they made.  Add them to CC for review.
>>
> 
> This might re-introduce a race condition if the code that is handling
> Tx done sends another byte without acknowledging the original interrupt,
> and another Tx done (or Tx nack) interrupt is received before the interrupt
> handler returns. If that happens, the second Tx done interrupt would be
> acknowledged but not be handled, and transmit would stall. That may well be
> what I had observed at the time but it is too long ago to remember, sorry.
> 
My assumption is that HW will start sending another byte as soon as the 
Tx done of the previous byte is ack'ed. So if it was ack'ed early before 
the slave irq handler to actually prepare the next byte to send, garbage 
value might be sent out instead.
I'm not sure if my assumption is correct but this patch do help in this 
case.
Please help with your comment.

>> Again, this is a fix that is independent of the ssif work. Please send
>> it separately with a Fixes line.
>>
>>>
>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>>> ---
>>> v3:
>>>    + First introduce in v3 [Quan]
>>>
>>>   drivers/i2c/busses/i2c-aspeed.c | 26 ++++++++++++++++++--------
>>>   1 file changed, 18 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c 
>>> b/drivers/i2c/busses/i2c-aspeed.c
>>> index 3fb37c3f23d4..b2e9c8f0ddf7 100644
>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>> @@ -606,8 +606,12 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, 
>>> void *dev_id)
>>>
>>>          spin_lock(&bus->lock);
>>>          irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>> -       /* Ack all interrupts except for Rx done */
>>> -       writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
>>> +       /*
>>> +        * Ack all interrupts except for Rx done and
>>> +        * Tx done with/without ACK
>>
>> Nit: this comment can be on one line.
>>
>>
>>> +        */
>>> +       writel(irq_received &
>>> +              ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | 
>>> ASPEED_I2CD_INTR_TX_NAK),
>>>                 bus->base + ASPEED_I2C_INTR_STS_REG);
>>>          readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>          irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
>>> @@ -652,12 +656,18 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, 
>>> void *dev_id)
>>>                          "irq handled != irq. expected 0x%08x, but 
>>> was 0x%08x\n",
>>>                          irq_received, irq_handled);
>>>
>>> -       /* Ack Rx done */
>>> -       if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
>>> -               writel(ASPEED_I2CD_INTR_RX_DONE,
>>> -                      bus->base + ASPEED_I2C_INTR_STS_REG);
>>> -               readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>> -       }
>>> +       /* Ack Rx done and Tx done with/without ACK */
>>> +       /* Note: Re-use irq_handled variable */
>>
>> I'm not sure what this note means.
>>
>>> +       irq_handled = 0;
>>> +       if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
>>> +               irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
>>> +       if (irq_received & ASPEED_I2CD_INTR_TX_ACK)
>>> +               irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>>> +       if (irq_received & ASPEED_I2CD_INTR_TX_NAK)
>>> +               irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>>> +       writel(irq_handled, bus->base + ASPEED_I2C_INTR_STS_REG);
>>
>> Are you intentionally only acking the bits that are set when we read
>> from STS_REG at the start of the handler? If not, we could write this
>> instead:
>>
>> writel(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK |
>> ASPEED_I2CD_INTR_TX_NAK,
>>          bus->base + ASPEED_I2C_INTR_STS_REG);
>>
> 
> This would clear those bits unconditionally even if they were not handled.
> 
>> If you only want to ack the bits that are set, then do this:
>>
>>    writel(irq_received &
>>              (ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK |
>> ASPEED_I2CD_INTR_TX_NAK),
>>           bus->base + ASPEED_I2C_INTR_STS_REG);
>>
>> That way, you can avoid all of the tests.
>>
> Or
>      irq_handled = irq_received &
>          (ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | 
> ASPEED_I2CD_INTR_TX_NAK);
>      writel(irq_handled, bus->base + ASPEED_I2C_INTR_STS_REG);
> 
> if the idea was to avoid the long statement.
> 
> Guenter
> 
Thanks Guenter,
Will apply in next version.

>>> +       readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>
>> When you move this, please add a comment that reminds us why we do a
>> write-then-read (see commit c926c87b8e36dcc0ea5c2a0a0227ed4f32d0516a).
>>
>>> +
>>>          spin_unlock(&bus->lock);
>>>          return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>   }
>>> -- 
>>> 2.28.0
>>>
> 

Re: [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late

[Quan Nguyen]

On 20/05/2021 06:43, Joel Stanley wrote:
> On Wed, 19 May 2021 at 07:50, Quan Nguyen <quan@os.amperecomputing.com> wrote:
>>
>> With Tx done w/wo ACK are ack'ed early at beginning of irq handler,
> 
> Is w/wo a typo? If not, please write the full words ("with and without")
> 
It is "with and without", will fix in next version

>> it is observed that, usually, the Tx done with Ack irq raises in the
>> READ REQUESTED state. This is unexpected and complaint as below appear:
>> "Unexpected Ack on read request"
>>
>> Assumed that Tx done should only be ack'ed once it was truly processed,
>> switch to late ack'ed this two irqs and seen this issue go away through
>> test with AST2500..
> 
> Please read Guneter's commit message
> 2be6b47211e17e6c90ead40d24d2a5cc815f2d5c to confirm that your changes
> do not invalidate the fix that they made.  Add them to CC for review.
> 
> Again, this is a fix that is independent of the ssif work. Please send
> it separately with a Fixes line.
> 
Will do and separate this patch into other series in next version.

>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + First introduce in v3 [Quan]
>>
>>   drivers/i2c/busses/i2c-aspeed.c | 26 ++++++++++++++++++--------
>>   1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 3fb37c3f23d4..b2e9c8f0ddf7 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -606,8 +606,12 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>
>>          spin_lock(&bus->lock);
>>          irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> -       /* Ack all interrupts except for Rx done */
>> -       writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
>> +       /*
>> +        * Ack all interrupts except for Rx done and
>> +        * Tx done with/without ACK
> 
> Nit: this comment can be on one line.
> 
Thanks, will fix.
> 
>> +        */
>> +       writel(irq_received &
>> +              ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK),
>>                 bus->base + ASPEED_I2C_INTR_STS_REG);
>>          readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>          irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
>> @@ -652,12 +656,18 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>                          "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>                          irq_received, irq_handled);
>>
>> -       /* Ack Rx done */
>> -       if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
>> -               writel(ASPEED_I2CD_INTR_RX_DONE,
>> -                      bus->base + ASPEED_I2C_INTR_STS_REG);
>> -               readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> -       }
>> +       /* Ack Rx done and Tx done with/without ACK */
>> +       /* Note: Re-use irq_handled variable */
> 
> I'm not sure what this note means.
> 
>> +       irq_handled = 0;
>> +       if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
>> +               irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
>> +       if (irq_received & ASPEED_I2CD_INTR_TX_ACK)
>> +               irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>> +       if (irq_received & ASPEED_I2CD_INTR_TX_NAK)
>> +               irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>> +       writel(irq_handled, bus->base + ASPEED_I2C_INTR_STS_REG);
> 
> Are you intentionally only acking the bits that are set when we read
> from STS_REG at the start of the handler? If not, we could write this
> instead:
> 
> writel(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK |
> ASPEED_I2CD_INTR_TX_NAK,
>          bus->base + ASPEED_I2C_INTR_STS_REG);
> 
> If you only want to ack the bits that are set, then do this:
> 
>    writel(irq_received &
>              (ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK |
> ASPEED_I2CD_INTR_TX_NAK),
>           bus->base + ASPEED_I2C_INTR_STS_REG);
> 
> That way, you can avoid all of the tests.
> 
Thanks, will fix this in next version.

>> +       readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> 
> When you move this, please add a comment that reminds us why we do a
> write-then-read (see commit c926c87b8e36dcc0ea5c2a0a0227ed4f32d0516a).
> 
Will fix in next version.
>> +
>>          spin_unlock(&bus->lock);
>>          return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>   }
>> --
>> 2.28.0
>>

[PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Quan Nguyen]

Slave i2c device on AST2500 received a lot of slave irq while it is
busy processing the response. To handle this case, adds and exports
aspeed_set_slave_busy() for controller to temporary stop slave irq
while slave is handling the response, and re-enable them again when
the response is ready.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
v3:
  + First introduce in v3 [Quan]

 drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index b2e9c8f0ddf7..9926d04831a2 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy)
+{
+	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
+	unsigned long current_mask, flags;
+
+	spin_lock_irqsave(&bus->lock, flags);
+
+	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
+	if (busy)
+		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_SLAVE_MATCH);
+	else
+		current_mask |= ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_SLAVE_MATCH;
+	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
+
+	spin_unlock_irqrestore(&bus->lock, flags);
+}
+EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);
+#endif
+
 static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)
 {
 	struct platform_device *pdev = to_platform_device(bus->dev);
-- 
2.28.0

RE: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Ryan Chen]

> -----Original Message-----
> From: openbmc
> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf
> Of Quan Nguyen
> Sent: Wednesday, May 19, 2021 3:50 PM
> To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;
> Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan
> Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> linux-i2c@vger.kernel.org
> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> 
> Slave i2c device on AST2500 received a lot of slave irq while it is busy
> processing the response. To handle this case, adds and exports
> aspeed_set_slave_busy() for controller to temporary stop slave irq while slave
> is handling the response, and re-enable them again when the response is ready.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + First introduce in v3 [Quan]
> 
>  drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index b2e9c8f0ddf7..9926d04831a2 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus
> *bus,
>  	return 0;
>  }
> 
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
> +	unsigned long current_mask, flags;
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +
> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +	if (busy)
> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE |
> ASPEED_I2CD_INTR_SLAVE_MATCH);
> +	else
> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE |
> ASPEED_I2CD_INTR_SLAVE_MATCH;
> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +
> +	spin_unlock_irqrestore(&bus->lock, flags); }
> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);
> +#endif
> +
>  static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)  {
>  	struct platform_device *pdev = to_platform_device(bus->dev);
> --
> 2.28.0

Hello,
	The better idea is use disable i2c slave mode. 
	Due to if i2c controller running in slave will get slave match, and latch the SCL.
	Until cpu clear interrupt status. 
Ryan

Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Quan Nguyen]

On 20/05/2021 18:06, Ryan Chen wrote:
>> -----Original Message-----
>> From: openbmc
>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf
>> Of Quan Nguyen
>> Sent: Wednesday, May 19, 2021 3:50 PM
>> To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;
>> Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan
>> Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>> linux-i2c@vger.kernel.org
>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>
>> Slave i2c device on AST2500 received a lot of slave irq while it is busy
>> processing the response. To handle this case, adds and exports
>> aspeed_set_slave_busy() for controller to temporary stop slave irq while slave
>> is handling the response, and re-enable them again when the response is ready.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + First introduce in v3 [Quan]
>>
>>   drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index b2e9c8f0ddf7..9926d04831a2 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus
>> *bus,
>>   	return 0;
>>   }
>>
>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
>> +	unsigned long current_mask, flags;
>> +
>> +	spin_lock_irqsave(&bus->lock, flags);
>> +
>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
>> +	if (busy)
>> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE |
>> ASPEED_I2CD_INTR_SLAVE_MATCH);
>> +	else
>> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE |
>> ASPEED_I2CD_INTR_SLAVE_MATCH;
>> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
>> +
>> +	spin_unlock_irqrestore(&bus->lock, flags); }
>> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);
>> +#endif
>> +
>>   static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)  {
>>   	struct platform_device *pdev = to_platform_device(bus->dev);
>> --
>> 2.28.0
> 
> Hello,
> 	The better idea is use disable i2c slave mode.
> 	Due to if i2c controller running in slave will get slave match, and latch the SCL.
> 	Until cpu clear interrupt status.
> Ryan
> 
Thanks Ryan,

Do you mean to enable/disable slave function as per example code below ?

         /* Turn on slave mode. */
         func_ctrl_reg_val = readl(bus->base + ASPEED_I2C_FUN_CTRL_REG);
         func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;
         writel(func_ctrl_reg_val, bus->base + ASPEED_I2C_FUN_CTRL_REG);

Will try this idea.
- Quan

RE: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Ryan Chen]

> -----Original Message-----
> From: Quan Nguyen <quan@os.amperecomputing.com>
> Sent: Thursday, May 20, 2021 10:10 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
> <brendanhiggins@google.com>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> linux-i2c@vger.kernel.org
> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> 
> On 20/05/2021 18:06, Ryan Chen wrote:
> >> -----Original Message-----
> >> From: openbmc
> >> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On
> Behalf
> >> Of Quan Nguyen
> >> Sent: Wednesday, May 19, 2021 3:50 PM
> >> To: Corey Minyard <minyard@acm.org>; Rob Herring
> >> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery
> >> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;
> >> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang
> >> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;
> >> openipmi-developer@lists.sourceforge.net;
> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> >> linux-i2c@vger.kernel.org
> >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
> >> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> >> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> >>
> >> Slave i2c device on AST2500 received a lot of slave irq while it is
> >> busy processing the response. To handle this case, adds and exports
> >> aspeed_set_slave_busy() for controller to temporary stop slave irq
> >> while slave is handling the response, and re-enable them again when the
> response is ready.
> >>
> >> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> >> ---
> >> v3:
> >>    + First introduce in v3 [Quan]
> >>
> >>   drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
> >>   1 file changed, 20 insertions(+)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c
> >> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2
> >> 100644
> >> --- a/drivers/i2c/busses/i2c-aspeed.c
> >> +++ b/drivers/i2c/busses/i2c-aspeed.c
> >> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus
> >> *bus,
> >>   	return 0;
> >>   }
> >>
> >> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> >> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
> >> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
> >> +	unsigned long current_mask, flags;
> >> +
> >> +	spin_lock_irqsave(&bus->lock, flags);
> >> +
> >> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> >> +	if (busy)
> >> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE |
> >> ASPEED_I2CD_INTR_SLAVE_MATCH);
> >> +	else
> >> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE |
> >> ASPEED_I2CD_INTR_SLAVE_MATCH;
> >> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
> >> +
> >> +	spin_unlock_irqrestore(&bus->lock, flags); }
> >> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);
> >> +#endif
> >> +
> >>   static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)  {
> >>   	struct platform_device *pdev = to_platform_device(bus->dev);
> >> --
> >> 2.28.0
> >
> > Hello,
> > 	The better idea is use disable i2c slave mode.
> > 	Due to if i2c controller running in slave will get slave match, and latch the
> SCL.
> > 	Until cpu clear interrupt status.
> > Ryan
> >
> Thanks Ryan,
> 
> Do you mean to enable/disable slave function as per example code below ?
Yes. it is.
> 
>          /* Turn on slave mode. */
>          func_ctrl_reg_val = readl(bus->base +
> ASPEED_I2C_FUN_CTRL_REG);
>          func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;
>          writel(func_ctrl_reg_val, bus->base +
> ASPEED_I2C_FUN_CTRL_REG);
> 
> Will try this idea.
> - Quan

Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Quan Nguyen]

On 21/05/2021 13:09, Ryan Chen wrote:
>> -----Original Message-----
>> From: Quan Nguyen <quan@os.amperecomputing.com>
>> Sent: Thursday, May 20, 2021 10:10 PM
>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
>> <brendanhiggins@google.com>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>> linux-i2c@vger.kernel.org
>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>
>> On 20/05/2021 18:06, Ryan Chen wrote:
>>>> -----Original Message-----
>>>> From: openbmc
>>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On
>> Behalf
>>>> Of Quan Nguyen
>>>> Sent: Wednesday, May 19, 2021 3:50 PM
>>>> To: Corey Minyard <minyard@acm.org>; Rob Herring
>>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery
>>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;
>>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang
>>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;
>>>> openipmi-developer@lists.sourceforge.net;
>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>>>> linux-i2c@vger.kernel.org
>>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>>>
>>>> Slave i2c device on AST2500 received a lot of slave irq while it is
>>>> busy processing the response. To handle this case, adds and exports
>>>> aspeed_set_slave_busy() for controller to temporary stop slave irq
>>>> while slave is handling the response, and re-enable them again when the
>> response is ready.
>>>>
>>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>>>> ---
>>>> v3:
>>>>     + First introduce in v3 [Quan]
>>>>
>>>>    drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
>>>>    1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2
>>>> 100644
>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus
>>>> *bus,
>>>>    	return 0;
>>>>    }
>>>>
>>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
>>>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
>>>> +	unsigned long current_mask, flags;
>>>> +
>>>> +	spin_lock_irqsave(&bus->lock, flags);
>>>> +
>>>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
>>>> +	if (busy)
>>>> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE |
>>>> ASPEED_I2CD_INTR_SLAVE_MATCH);
>>>> +	else
>>>> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE |
>>>> ASPEED_I2CD_INTR_SLAVE_MATCH;
>>>> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
>>>> +
>>>> +	spin_unlock_irqrestore(&bus->lock, flags); }
>>>> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);
>>>> +#endif
>>>> +
>>>>    static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)  {
>>>>    	struct platform_device *pdev = to_platform_device(bus->dev);
>>>> --
>>>> 2.28.0
>>>
>>> Hello,
>>> 	The better idea is use disable i2c slave mode.
>>> 	Due to if i2c controller running in slave will get slave match, and latch the
>> SCL.
>>> 	Until cpu clear interrupt status.
>>> Ryan
>>>
>> Thanks Ryan,
>>
>> Do you mean to enable/disable slave function as per example code below ?
> Yes. it is.

Dear Ryan,

This solution looks good. I'll switch to use this way in next version.
Thanks for the suggestion.

- Quan

>>
>>           /* Turn on slave mode. */
>>           func_ctrl_reg_val = readl(bus->base +
>> ASPEED_I2C_FUN_CTRL_REG);
>>           func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;
>>           writel(func_ctrl_reg_val, bus->base +
>> ASPEED_I2C_FUN_CTRL_REG);
>>
>> Will try this idea.
>> - Quan

RE: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Ryan Chen]

> -----Original Message-----
> From: openbmc
> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf
> Of Quan Nguyen
> Sent: Wednesday, May 19, 2021 3:50 PM
> To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;
> Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan
> Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> linux-i2c@vger.kernel.org
> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> 
> Slave i2c device on AST2500 received a lot of slave irq while it is busy
> processing the response. To handle this case, adds and exports
> aspeed_set_slave_busy() for controller to temporary stop slave irq while slave
> is handling the response, and re-enable them again when the response is ready.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + First introduce in v3 [Quan]
> 
>  drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index b2e9c8f0ddf7..9926d04831a2 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus
> *bus,
>  	return 0;
>  }
> 
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
> +	unsigned long current_mask, flags;
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +
> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
Hello 
	Where the bus->base to be remap?

> +	if (busy)
> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE |
> ASPEED_I2CD_INTR_SLAVE_MATCH);
> +	else
> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE |
> ASPEED_I2CD_INTR_SLAVE_MATCH;
> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +
> +	spin_unlock_irqrestore(&bus->lock, flags); }
> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);
> +#endif
> +
>  static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)  {
>  	struct platform_device *pdev = to_platform_device(bus->dev);
> --
> 2.28.0

Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Quan Nguyen]

On 24/05/2021 17:06, Ryan Chen wrote:
>> -----Original Message-----
>> From: openbmc
>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf
>> Of Quan Nguyen
>> Sent: Wednesday, May 19, 2021 3:50 PM
>> To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;
>> Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan
>> Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>> linux-i2c@vger.kernel.org
>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>
>> Slave i2c device on AST2500 received a lot of slave irq while it is busy
>> processing the response. To handle this case, adds and exports
>> aspeed_set_slave_busy() for controller to temporary stop slave irq while slave
>> is handling the response, and re-enable them again when the response is ready.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + First introduce in v3 [Quan]
>>
>>   drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index b2e9c8f0ddf7..9926d04831a2 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus
>> *bus,
>>   	return 0;
>>   }
>>
>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
>> +	unsigned long current_mask, flags;
>> +
>> +	spin_lock_irqsave(&bus->lock, flags);
>> +
>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> Hello
> 	Where the bus->base to be remap?
> 

Hi Ryan,

In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the 
->priv is retrieved by calling i2c_get_adapdata(client->adapter). And in 
aspeed_set_ssif_bmc_status(), call the exported aspeed_set_slave_busy() 
using ->priv pointer as code below.

+extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy);
+static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc, 
unsigned int status)
+{
+	if (status & SSIF_BMC_BUSY)
+		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true);
+	else if (status & SSIF_BMC_READY)
+		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false);
+}
+
+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, 0);
+	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;
+}

- Quan

RE: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Ryan Chen]

> -----Original Message-----
> From: Quan Nguyen <quan@os.amperecomputing.com>
> Sent: Monday, May 24, 2021 6:20 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
> <brendanhiggins@google.com>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> linux-i2c@vger.kernel.org
> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> 
> On 24/05/2021 17:06, Ryan Chen wrote:
> >> -----Original Message-----
> >> From: openbmc
> >> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On
> Behalf
> >> Of Quan Nguyen
> >> Sent: Wednesday, May 19, 2021 3:50 PM
> >> To: Corey Minyard <minyard@acm.org>; Rob Herring
> >> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery
> >> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;
> >> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang
> >> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;
> >> openipmi-developer@lists.sourceforge.net;
> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> >> linux-i2c@vger.kernel.org
> >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
> >> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> >> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> >>
> >> Slave i2c device on AST2500 received a lot of slave irq while it is
> >> busy processing the response. To handle this case, adds and exports
> >> aspeed_set_slave_busy() for controller to temporary stop slave irq
> >> while slave is handling the response, and re-enable them again when the
> response is ready.
> >>
> >> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> >> ---
> >> v3:
> >>    + First introduce in v3 [Quan]
> >>
> >>   drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
> >>   1 file changed, 20 insertions(+)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c
> >> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2
> >> 100644
> >> --- a/drivers/i2c/busses/i2c-aspeed.c
> >> +++ b/drivers/i2c/busses/i2c-aspeed.c
> >> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus
> >> *bus,
> >>   	return 0;
> >>   }
> >>
> >> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> >> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
> >> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
> >> +	unsigned long current_mask, flags;
> >> +
> >> +	spin_lock_irqsave(&bus->lock, flags);
> >> +
> >> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> > Hello
> > 	Where the bus->base to be remap?
> >
> 
> Hi Ryan,
> 
> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the
> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And in
> aspeed_set_ssif_bmc_status(), call the exported aspeed_set_slave_busy()
> using ->priv pointer as code below.
> 
Yes, I see the probe function " ssif_bmc->priv = i2c_get_adapdata(client->adapter);" to get priv.
But my question I don’t see the bus->base address be assigned. 

> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy);
> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc,
> unsigned int status)
> +{
> +	if (status & SSIF_BMC_BUSY)
> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true);
> +	else if (status & SSIF_BMC_READY)
> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); }
> +
> +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, 0);
> +	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;
> +}
> 
> - Quan
> 
> 

Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Quan Nguyen]

On 24/05/2021 17:36, Ryan Chen wrote:
>> -----Original Message-----
>> From: Quan Nguyen <quan@os.amperecomputing.com>
>> Sent: Monday, May 24, 2021 6:20 PM
>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
>> <brendanhiggins@google.com>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>> linux-i2c@vger.kernel.org
>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>
>> On 24/05/2021 17:06, Ryan Chen wrote:
>>>> -----Original Message-----
>>>> From: openbmc
>>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On
>> Behalf
>>>> Of Quan Nguyen
>>>> Sent: Wednesday, May 19, 2021 3:50 PM
>>>> To: Corey Minyard <minyard@acm.org>; Rob Herring
>>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery
>>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;
>>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang
>>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;
>>>> openipmi-developer@lists.sourceforge.net;
>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>>>> linux-i2c@vger.kernel.org
>>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>>>
>>>> Slave i2c device on AST2500 received a lot of slave irq while it is
>>>> busy processing the response. To handle this case, adds and exports
>>>> aspeed_set_slave_busy() for controller to temporary stop slave irq
>>>> while slave is handling the response, and re-enable them again when the
>> response is ready.
>>>>
>>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>>>> ---
>>>> v3:
>>>>     + First introduce in v3 [Quan]
>>>>
>>>>    drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
>>>>    1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2
>>>> 100644
>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus
>>>> *bus,
>>>>    	return 0;
>>>>    }
>>>>
>>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
>>>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
>>>> +	unsigned long current_mask, flags;
>>>> +
>>>> +	spin_lock_irqsave(&bus->lock, flags);
>>>> +
>>>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
>>> Hello
>>> 	Where the bus->base to be remap?
>>>
>>
>> Hi Ryan,
>>
>> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the
>> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And in
>> aspeed_set_ssif_bmc_status(), call the exported aspeed_set_slave_busy()
>> using ->priv pointer as code below.
>>
> Yes, I see the probe function " ssif_bmc->priv = i2c_get_adapdata(client->adapter);" to get priv.
> But my question I don’t see the bus->base address be assigned.
> 
Hi Ryan,

In drivers/i2c/busses/i2c-aspeed.c:
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;

So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the 
bus->base should point to the base of the aspeed_i2c_bus, which is 
already initialized by the aspeed i2c bus driver.

Do I miss something?

- Quan


>> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy);
>> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc,
>> unsigned int status)
>> +{
>> +	if (status & SSIF_BMC_BUSY)
>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true);
>> +	else if (status & SSIF_BMC_READY)
>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); }
>> +
>> +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, 0);
>> +	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;
>> +}
>>
>> - Quan
>>
>>
> 

RE: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Ryan Chen]

> -----Original Message-----
> From: Quan Nguyen <quan@os.amperecomputing.com>
> Sent: Monday, May 24, 2021 6:49 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
> <brendanhiggins@google.com>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> linux-i2c@vger.kernel.org
> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> 
> On 24/05/2021 17:36, Ryan Chen wrote:
> >> -----Original Message-----
> >> From: Quan Nguyen <quan@os.amperecomputing.com>
> >> Sent: Monday, May 24, 2021 6:20 PM
> >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
> >> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
> >> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
> >> <brendanhiggins@google.com>; Benjamin Herrenschmidt
> >> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp
> >> Zabel <p.zabel@pengutronix.de>;
> >> openipmi-developer@lists.sourceforge.net;
> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> >> linux-i2c@vger.kernel.org
> >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
> >> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> >> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> >>
> >> On 24/05/2021 17:06, Ryan Chen wrote:
> >>>> -----Original Message-----
> >>>> From: openbmc
> >>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On
> >> Behalf
> >>>> Of Quan Nguyen
> >>>> Sent: Wednesday, May 19, 2021 3:50 PM
> >>>> To: Corey Minyard <minyard@acm.org>; Rob Herring
> >>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery
> >>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;
> >>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang
> >>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;
> >>>> openipmi-developer@lists.sourceforge.net;
> >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> >>>> linux-i2c@vger.kernel.org
> >>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang
> Q .
> >>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> >>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> >>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> >>>>
> >>>> Slave i2c device on AST2500 received a lot of slave irq while it is
> >>>> busy processing the response. To handle this case, adds and exports
> >>>> aspeed_set_slave_busy() for controller to temporary stop slave irq
> >>>> while slave is handling the response, and re-enable them again when
> >>>> the
> >> response is ready.
> >>>>
> >>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> >>>> ---
> >>>> v3:
> >>>>     + First introduce in v3 [Quan]
> >>>>
> >>>>    drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
> >>>>    1 file changed, 20 insertions(+)
> >>>>
> >>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
> >>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2
> >>>> 100644
> >>>> --- a/drivers/i2c/busses/i2c-aspeed.c
> >>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
> >>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct
> >>>> aspeed_i2c_bus *bus,
> >>>>    	return 0;
> >>>>    }
> >>>>
> >>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> >>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
> >>>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
> >>>> +	unsigned long current_mask, flags;
> >>>> +
> >>>> +	spin_lock_irqsave(&bus->lock, flags);
> >>>> +
> >>>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> >>> Hello
> >>> 	Where the bus->base to be remap?
> >>>
> >>
> >> Hi Ryan,
> >>
> >> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the
> >> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And
> >> ->in
> >> aspeed_set_ssif_bmc_status(), call the exported
> >> aspeed_set_slave_busy() using ->priv pointer as code below.
> >>
> > Yes, I see the probe function " ssif_bmc->priv =
> i2c_get_adapdata(client->adapter);" to get priv.
> > But my question I don’t see the bus->base address be assigned.
> >
> Hi Ryan,
> 
> In drivers/i2c/busses/i2c-aspeed.c:
> 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;
> 
> So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the
> bus->base should point to the base of the aspeed_i2c_bus, which is
> already initialized by the aspeed i2c bus driver.
> 
> Do I miss something?
Hello Quan,
	After study. I think the ssif_bmc_aspeed.c assume the priv data is the same struct.
	That is trick.
	Do we have a better for slave enable/disable call back to implement this?
	If add call back in struct i2c_algorithm; is it workable?
> 
> >> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy);
> >> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc,
> >> unsigned int status)
> >> +{
> >> +	if (status & SSIF_BMC_BUSY)
> >> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true);
> >> +	else if (status & SSIF_BMC_READY)
> >> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); }
> >> +
> >> +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, 0);
> >> +	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;
> >> +}
> >>
> >> - Quan
> >>
> >>
> >

Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Quan Nguyen]

On 25/05/2021 17:30, Ryan Chen wrote:
>> -----Original Message-----
>> From: Quan Nguyen <quan@os.amperecomputing.com>
>> Sent: Monday, May 24, 2021 6:49 PM
>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
>> <brendanhiggins@google.com>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>> linux-i2c@vger.kernel.org
>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>
>> On 24/05/2021 17:36, Ryan Chen wrote:
>>>> -----Original Message-----
>>>> From: Quan Nguyen <quan@os.amperecomputing.com>
>>>> Sent: Monday, May 24, 2021 6:20 PM
>>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
>>>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
>>>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
>>>> <brendanhiggins@google.com>; Benjamin Herrenschmidt
>>>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp
>>>> Zabel <p.zabel@pengutronix.de>;
>>>> openipmi-developer@lists.sourceforge.net;
>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>>>> linux-i2c@vger.kernel.org
>>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>>>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>>>
>>>> On 24/05/2021 17:06, Ryan Chen wrote:
>>>>>> -----Original Message-----
>>>>>> From: openbmc
>>>>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On
>>>> Behalf
>>>>>> Of Quan Nguyen
>>>>>> Sent: Wednesday, May 19, 2021 3:50 PM
>>>>>> To: Corey Minyard <minyard@acm.org>; Rob Herring
>>>>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery
>>>>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;
>>>>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang
>>>>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;
>>>>>> openipmi-developer@lists.sourceforge.net;
>>>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>>>>>> linux-i2c@vger.kernel.org
>>>>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang
>> Q .
>>>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>>>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>>>>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>>>>>
>>>>>> Slave i2c device on AST2500 received a lot of slave irq while it is
>>>>>> busy processing the response. To handle this case, adds and exports
>>>>>> aspeed_set_slave_busy() for controller to temporary stop slave irq
>>>>>> while slave is handling the response, and re-enable them again when
>>>>>> the
>>>> response is ready.
>>>>>>
>>>>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>>>>>> ---
>>>>>> v3:
>>>>>>      + First introduce in v3 [Quan]
>>>>>>
>>>>>>     drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
>>>>>>     1 file changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>>>>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2
>>>>>> 100644
>>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct
>>>>>> aspeed_i2c_bus *bus,
>>>>>>     	return 0;
>>>>>>     }
>>>>>>
>>>>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
>>>>>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
>>>>>> +	unsigned long current_mask, flags;
>>>>>> +
>>>>>> +	spin_lock_irqsave(&bus->lock, flags);
>>>>>> +
>>>>>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
>>>>> Hello
>>>>> 	Where the bus->base to be remap?
>>>>>
>>>>
>>>> Hi Ryan,
>>>>
>>>> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the
>>>> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And
>>>> ->in
>>>> aspeed_set_ssif_bmc_status(), call the exported
>>>> aspeed_set_slave_busy() using ->priv pointer as code below.
>>>>
>>> Yes, I see the probe function " ssif_bmc->priv =
>> i2c_get_adapdata(client->adapter);" to get priv.
>>> But my question I don’t see the bus->base address be assigned.
>>>
>> Hi Ryan,
>>
>> In drivers/i2c/busses/i2c-aspeed.c:
>> 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;
>>
>> So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the
>> bus->base should point to the base of the aspeed_i2c_bus, which is
>> already initialized by the aspeed i2c bus driver.
>>
>> Do I miss something?
> Hello Quan,
> 	After study. I think the ssif_bmc_aspeed.c assume the priv data is the same struct.
> 	That is trick.
> 	Do we have a better for slave enable/disable call back to implement this?
> 	If add call back in struct i2c_algorithm; is it workable?
>>

Hi Ryan,

I dont know which is better, ie: adding callback to struct i2c_algorithm 
or to struct i2c_adapter.
I have tried to add generic callback to struct i2c_adapter as below and 
it works.

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 4e7714c88f95..6e9abf2d6abb 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -713,6 +713,10 @@ struct i2c_adapter {
  	const struct i2c_adapter_quirks *quirks;

  	struct irq_domain *host_notify_domain;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	int (*slave_enable)(struct i2c_adapter *adap);
+	int (*slave_disable)(struct i2c_adapter *adap);
+#endif
  };
  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)

>>>> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy);
>>>> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc,
>>>> unsigned int status)
>>>> +{
>>>> +	if (status & SSIF_BMC_BUSY)
>>>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true);
>>>> +	else if (status & SSIF_BMC_READY)
>>>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); }
>>>> +
>>>> +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, 0);
>>>> +	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;
>>>> +}
>>>>
>>>> - Quan
>>>>
>>>>
>>>
> 

RE: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Ryan Chen]

> -----Original Message-----
> From: Quan Nguyen <quan@os.amperecomputing.com>
> Sent: Friday, May 28, 2021 8:53 AM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
> <brendanhiggins@google.com>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> linux-i2c@vger.kernel.org
> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> 
> On 25/05/2021 17:30, Ryan Chen wrote:
> >> -----Original Message-----
> >> From: Quan Nguyen <quan@os.amperecomputing.com>
> >> Sent: Monday, May 24, 2021 6:49 PM
> >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
> >> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
> >> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
> >> <brendanhiggins@google.com>; Benjamin Herrenschmidt
> >> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp
> >> Zabel <p.zabel@pengutronix.de>;
> >> openipmi-developer@lists.sourceforge.net;
> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> >> linux-i2c@vger.kernel.org
> >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
> >> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> >> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> >>
> >> On 24/05/2021 17:36, Ryan Chen wrote:
> >>>> -----Original Message-----
> >>>> From: Quan Nguyen <quan@os.amperecomputing.com>
> >>>> Sent: Monday, May 24, 2021 6:20 PM
> >>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
> >>>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
> >>>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
> >>>> <brendanhiggins@google.com>; Benjamin Herrenschmidt
> >>>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp
> >>>> Zabel <p.zabel@pengutronix.de>;
> >>>> openipmi-developer@lists.sourceforge.net;
> >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> >>>> linux-i2c@vger.kernel.org
> >>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang
> Q .
> >>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> >>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> >>>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add
> >>>> aspeed_set_slave_busy()
> >>>>
> >>>> On 24/05/2021 17:06, Ryan Chen wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: openbmc
> >>>>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On
> >>>> Behalf
> >>>>>> Of Quan Nguyen
> >>>>>> Sent: Wednesday, May 19, 2021 3:50 PM
> >>>>>> To: Corey Minyard <minyard@acm.org>; Rob Herring
> >>>>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew
> >>>>>> Jeffery <andrew@aj.id.au>; Brendan Higgins
> >>>>>> <brendanhiggins@google.com>; Benjamin Herrenschmidt
> >>>>>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>;
> >>>>>> Philipp Zabel <p.zabel@pengutronix.de>;
> >>>>>> openipmi-developer@lists.sourceforge.net;
> >>>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >>>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> >>>>>> linux-i2c@vger.kernel.org
> >>>>>> Cc: Open Source Submission <patches@amperecomputing.com>;
> Thang
> >> Q .
> >>>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
> >>>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
> >>>>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
> >>>>>>
> >>>>>> Slave i2c device on AST2500 received a lot of slave irq while it
> >>>>>> is busy processing the response. To handle this case, adds and
> >>>>>> exports
> >>>>>> aspeed_set_slave_busy() for controller to temporary stop slave
> >>>>>> irq while slave is handling the response, and re-enable them
> >>>>>> again when the
> >>>> response is ready.
> >>>>>>
> >>>>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> >>>>>> ---
> >>>>>> v3:
> >>>>>>      + First introduce in v3 [Quan]
> >>>>>>
> >>>>>>     drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
> >>>>>>     1 file changed, 20 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
> >>>>>> b/drivers/i2c/busses/i2c-aspeed.c index
> >>>>>> b2e9c8f0ddf7..9926d04831a2
> >>>>>> 100644
> >>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
> >>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
> >>>>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct
> >>>>>> aspeed_i2c_bus *bus,
> >>>>>>     	return 0;
> >>>>>>     }
> >>>>>>
> >>>>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE) void
> >>>>>> +aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
> >>>>>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
> >>>>>> +	unsigned long current_mask, flags;
> >>>>>> +
> >>>>>> +	spin_lock_irqsave(&bus->lock, flags);
> >>>>>> +
> >>>>>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> >>>>> Hello
> >>>>> 	Where the bus->base to be remap?
> >>>>>
> >>>>
> >>>> Hi Ryan,
> >>>>
> >>>> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the
> >>>> ->priv is retrieved by calling i2c_get_adapdata(client->adapter).
> >>>> ->And in
> >>>> aspeed_set_ssif_bmc_status(), call the exported
> >>>> aspeed_set_slave_busy() using ->priv pointer as code below.
> >>>>
> >>> Yes, I see the probe function " ssif_bmc->priv =
> >> i2c_get_adapdata(client->adapter);" to get priv.
> >>> But my question I don’t see the bus->base address be assigned.
> >>>
> >> Hi Ryan,
> >>
> >> In drivers/i2c/busses/i2c-aspeed.c:
> >> 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;
> >>
> >> So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the
> >> bus->base should point to the base of the aspeed_i2c_bus, which is
> >> already initialized by the aspeed i2c bus driver.
> >>
> >> Do I miss something?
> > Hello Quan,
> > 	After study. I think the ssif_bmc_aspeed.c assume the priv data is the
> same struct.
> > 	That is trick.
> > 	Do we have a better for slave enable/disable call back to implement this?
> > 	If add call back in struct i2c_algorithm; is it workable?
> >>
> 
> Hi Ryan,
> 
> I dont know which is better, ie: adding callback to struct i2c_algorithm or to
> struct i2c_adapter.
> I have tried to add generic callback to struct i2c_adapter as below and it
> works.
> 
Hello Quan,
	Thanks your feedback.
	Because, if we apply it in callback. It can unify it in ssif_bmc.c to do callback.
	Not have ssif_bmc_aspeed.c, ssif_bmc_xxx.c .....
	Because the priv struct is different in different i2c driver implement. 
Ryan
	

> diff --git a/include/linux/i2c.h b/include/linux/i2c.h index
> 4e7714c88f95..6e9abf2d6abb 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -713,6 +713,10 @@ struct i2c_adapter {
>   	const struct i2c_adapter_quirks *quirks;
> 
>   	struct irq_domain *host_notify_domain;
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	int (*slave_enable)(struct i2c_adapter *adap);
> +	int (*slave_disable)(struct i2c_adapter *adap); #endif
>   };
>   #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
> 
> >>>> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool
> >>>> +busy); static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx
> >>>> +*ssif_bmc,
> >>>> unsigned int status)
> >>>> +{
> >>>> +	if (status & SSIF_BMC_BUSY)
> >>>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv,
> true);
> >>>> +	else if (status & SSIF_BMC_READY)
> >>>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv,
> >>>> +false); }
> >>>> +
> >>>> +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, 0);
> >>>> +	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;
> >>>> +}
> >>>>
> >>>> - Quan
> >>>>
> >>>>
> >>>
> >

Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

[Graeme Gregory]

On Wed, May 19, 2021 at 02:49:32PM +0700, Quan Nguyen wrote:
> Slave i2c device on AST2500 received a lot of slave irq while it is
> busy processing the response. To handle this case, adds and exports
> aspeed_set_slave_busy() for controller to temporary stop slave irq
> while slave is handling the response, and re-enable them again when
> the response is ready.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + First introduce in v3 [Quan]
> 
>  drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index b2e9c8f0ddf7..9926d04831a2 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy)
> +{
> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
> +	unsigned long current_mask, flags;
> +
> +	spin_lock_irqsave(&bus->lock, flags);

This as far as I can see is still a recursive spinlock, and the spinlock
debugger seems to agree with me.

Graeme

> +
> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +	if (busy)
> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_SLAVE_MATCH);
> +	else
> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_SLAVE_MATCH;
> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);
> +#endif
> +
>  static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)
>  {
>  	struct platform_device *pdev = to_platform_device(bus->dev);
> -- 
> 2.28.0
> 

[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver

[Quan Nguyen]

This commits adds SSIF BMC driver specifically for Aspeed AST2500 which
commonly used as Board Management Controllers.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
v3:
  + Splited into separate commit [Corey, Joel]
  + Invoked aspeed-specific aspeed_set_slave_busy() when busy
to address deadlock from Graeme and comment from Philipp
  + Combined two interrupts to mask/unmask together [Corey]

 drivers/char/ipmi/Kconfig           | 11 +++++
 drivers/char/ipmi/Makefile          |  1 +
 drivers/char/ipmi/ssif_bmc_aspeed.c | 75 +++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index ad5c5161bcd6..45be57023577 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -144,6 +144,17 @@ config SSIF_IPMI_BMC
 	  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 d04a214d74c4..05b993f7335b 100644
--- a/drivers/char/ipmi/Makefile
+++ b/drivers/char/ipmi/Makefile
@@ -28,3 +28,4 @@ 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_aspeed.c b/drivers/char/ipmi/ssif_bmc_aspeed.c
new file mode 100644
index 000000000000..019e51e16a7e
--- /dev/null
+++ b/drivers/char/ipmi/ssif_bmc_aspeed.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * The driver for BMC side of Aspeed SSIF interface
+ * Copyright (c) 2021, Ampere Computing LLC
+ */
+
+#include <linux/i2c.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/poll.h>
+#include <linux/iopoll.h>
+
+#include "ssif_bmc.h"
+
+extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy);
+static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc, unsigned int status)
+{
+	if (status & SSIF_BMC_BUSY)
+		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true);
+	else if (status & SSIF_BMC_READY)
+		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false);
+}
+
+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, 0);
+	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

[PATCH v3 7/7] bindings: ipmi: Add binding for Aspeed SSIF BMC driver

[Quan Nguyen]

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

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
v3:
  + Switched to use DT schema format [Rob]

 .../bindings/ipmi/aspeed-ssif-bmc.yaml        | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml

diff --git a/Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml b/Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
new file mode 100644
index 000000000000..8608041fbf7e
--- /dev/null
+++ b/Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/ipmi/aspeed-ssif-bmc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed SSIF IPMI interface
+
+description: SSIF IPMI device bindings for Aspeed
+
+maintainers:
+  - Thang Nguyen <thang@os.amperecomputing.com>
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2500-ssif-bmc
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    ssif-bmc@10 {
+        compatible = "aspeed,ast2500-ssif-bmc";
+        reg = <0x10>;
+    };
-- 
2.28.0

Re: [PATCH v3 7/7] bindings: ipmi: Add binding for Aspeed SSIF BMC driver

[Rob Herring]

On Wed, 19 May 2021 14:49:34 +0700, Quan Nguyen wrote:
> Add device tree binding document for the Aspeed SSIF BMC driver.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + Switched to use DT schema format [Rob]
> 
>  .../bindings/ipmi/aspeed-ssif-bmc.yaml        | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dts:21.13-26: Warning (reg_format): /example-0/ssif-bmc@10:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: example-0: ssif-bmc@10:reg:0: [16] is too short
	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml

See https://patchwork.ozlabs.org/patch/1480727

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 7/7] bindings: ipmi: Add binding for Aspeed SSIF BMC driver

[Quan Nguyen]

On 19/05/2021 22:29, Rob Herring wrote:
> On Wed, 19 May 2021 14:49:34 +0700, Quan Nguyen wrote:
>> Add device tree binding document for the Aspeed SSIF BMC driver.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + Switched to use DT schema format [Rob]
>>
>>   .../bindings/ipmi/aspeed-ssif-bmc.yaml        | 33 +++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
>>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dts:21.13-26: Warning (reg_format): /example-0/ssif-bmc@10:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1)
> Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (pci_device_reg): Failed prerequisite 'reg_format'
> Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
> Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
> Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
> Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: example-0: ssif-bmc@10:reg:0: [16] is too short
> 	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml
> 
> See https://patchwork.ozlabs.org/patch/1480727
> 
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit.
> 
Thanks,
Will fix in next version.

- Quan

Re: [PATCH v3 0/7] Add Aspeed SSIF BMC driver

[Corey Minyard]

On Wed, May 19, 2021 at 02:49:27PM +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.
> 
> v3:
>   + Switched binding doc to use DT schema format [Rob]
>   + Splited into generic ssif_bmc and aspeed-specific [Corey, Joel]
>   + Removed redundant license info [Joel]
>   + Switched to use traditional if-else [Joel]
>   + Removed unused ssif_bmc_ioctl() [Joel]
>   + Made handle_request()/complete_response() to return void [Joel]
>   + Refactored send_ssif_bmc_response()/receive_ssif_bmc_request() [Corey]
>   + Remove mutex [Corey]
>   + Use spin_lock/unlock_irqsave/restore in callback [Corey]
>   + Removed the unnecessary memset [Corey]
>   + Switch to use dev_err() [Corey]
>   + Combine mask/unmask two interrupts together [Corey]
>   + Fixed unhandled Tx done with NAK [Quan]
>   + Late ack'ed Tx done w/wo Ack irq [Quan]
>   + Use aspeed-specific exported aspeed_set_slave_busy() when slave busy
> to fix the deadlock [Graeme, Philipp, Quan]
>   + Clean buffer for last multipart read [Quan]
>   + Handle unknown incoming command [Quan]
> 
> v2:
>   + Fixed compiling error with COMPILE_TEST for arc
> 
> Quan Nguyen (7):
>   i2c: i2c-core-smbus: Expose PEC calculate function for generic use

Note that for this I2C-specific change, I will need acks from the I2C
maintainers to be able to include this in my tree.

>   ipmi: ssif_bmc: Add SSIF BMC driver
>   i2c: aspeed: Fix unhandled Tx done with NAK

For the aspeed changes, they don't really belong here, they belong in
the aspeed tree.  I see that you need them for the device to work
correctly, though.  I'll need acks from maintainers to include them.

>   i2c: aspeed: Acknowledge Tx done w/wo ACK irq late
>   i2c: aspeed: Add aspeed_set_slave_busy()
>   ipmi: ssif_bmc: Add Aspeed SSIF BMC driver
>   bindings: ipmi: Add binding for Aspeed SSIF BMC driver
> 
>  .../bindings/ipmi/aspeed-ssif-bmc.yaml        |  33 +
>  drivers/char/ipmi/Kconfig                     |  22 +
>  drivers/char/ipmi/Makefile                    |   2 +
>  drivers/char/ipmi/ssif_bmc.c                  | 605 ++++++++++++++++++
>  drivers/char/ipmi/ssif_bmc.h                  |  93 +++
>  drivers/char/ipmi/ssif_bmc_aspeed.c           |  75 +++
>  drivers/i2c/busses/i2c-aspeed.c               |  51 +-
>  drivers/i2c/i2c-core-smbus.c                  |  12 +-
>  include/linux/i2c.h                           |   1 +
>  9 files changed, 884 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
>  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
> 

Re: [PATCH v3 0/7] Add Aspeed SSIF BMC driver

[Quan Nguyen]

On 19/05/2021 19:34, Corey Minyard wrote:
> On Wed, May 19, 2021 at 02:49:27PM +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.
>>
>> v3:
>>    + Switched binding doc to use DT schema format [Rob]
>>    + Splited into generic ssif_bmc and aspeed-specific [Corey, Joel]
>>    + Removed redundant license info [Joel]
>>    + Switched to use traditional if-else [Joel]
>>    + Removed unused ssif_bmc_ioctl() [Joel]
>>    + Made handle_request()/complete_response() to return void [Joel]
>>    + Refactored send_ssif_bmc_response()/receive_ssif_bmc_request() [Corey]
>>    + Remove mutex [Corey]
>>    + Use spin_lock/unlock_irqsave/restore in callback [Corey]
>>    + Removed the unnecessary memset [Corey]
>>    + Switch to use dev_err() [Corey]
>>    + Combine mask/unmask two interrupts together [Corey]
>>    + Fixed unhandled Tx done with NAK [Quan]
>>    + Late ack'ed Tx done w/wo Ack irq [Quan]
>>    + Use aspeed-specific exported aspeed_set_slave_busy() when slave busy
>> to fix the deadlock [Graeme, Philipp, Quan]
>>    + Clean buffer for last multipart read [Quan]
>>    + Handle unknown incoming command [Quan]
>>
>> v2:
>>    + Fixed compiling error with COMPILE_TEST for arc
>>
>> Quan Nguyen (7):
>>    i2c: i2c-core-smbus: Expose PEC calculate function for generic use
> 
> Note that for this I2C-specific change, I will need acks from the I2C
> maintainers to be able to include this in my tree.
>  >>    ipmi: ssif_bmc: Add SSIF BMC driver
>>    i2c: aspeed: Fix unhandled Tx done with NAK
> 
> For the aspeed changes, they don't really belong here, they belong in
> the aspeed tree.  I see that you need them for the device to work
> correctly, though.  I'll need acks from maintainers to include them.
> 
Thanks for you information.
Will separate these i2c-aspeed's patches into other independent series 
in next version.

>>    i2c: aspeed: Acknowledge Tx done w/wo ACK irq late
>>    i2c: aspeed: Add aspeed_set_slave_busy()
>>    ipmi: ssif_bmc: Add Aspeed SSIF BMC driver
>>    bindings: ipmi: Add binding for Aspeed SSIF BMC driver
>>
>>   .../bindings/ipmi/aspeed-ssif-bmc.yaml        |  33 +
>>   drivers/char/ipmi/Kconfig                     |  22 +
>>   drivers/char/ipmi/Makefile                    |   2 +
>>   drivers/char/ipmi/ssif_bmc.c                  | 605 ++++++++++++++++++
>>   drivers/char/ipmi/ssif_bmc.h                  |  93 +++
>>   drivers/char/ipmi/ssif_bmc_aspeed.c           |  75 +++
>>   drivers/i2c/busses/i2c-aspeed.c               |  51 +-
>>   drivers/i2c/i2c-core-smbus.c                  |  12 +-
>>   include/linux/i2c.h                           |   1 +
>>   9 files changed, 884 insertions(+), 10 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
>>   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
>>