[PATCH 0/6] misc: add amd side-band functionality

[PATCH 0/6] misc: add amd side-band functionality

[Akshay Gupta]

AMD's APML interface provides system management functionality accessed by
the BMC. Sbrmi driver under hwmon subsystem, which is probed as an i2c
driver and reports power using APML specified protocol.

However, APML interface defines few other protocols to support
full system management functionality out-of-band.

This patchset is an attempt to add all APML core functionality in one place
and provide hwmon and user interface
1. Move the i2c client probe and sbrmi core functionality from drivers/hwmon
   to drivers/misc/
2. Add a platform device, which probes the hwmon/sbrmi and continues to
   report power using the symbol exported by the misc/sbrmi-core.
3. Convert i2c to regmap which provides multiple benefits
   over direct smbus APIs.
4. Register a misc device which provides 
    a. An ioctl interface through node /dev/sbrmiX
    b. Open-sourced and widely used https://github.com/amd/esmi_oob_library
       will continue to provide user-space programmable API.

Akshay Gupta (6):
  hwmon/misc sbrmi: Move core sbrmi from hwmon to misc
  misc: sbrmi: Add platform device add to create platform device
  misc: sbrmi: Use regmap subsystem
  misc: sbrmi: Clear sbrmi status register bit SwAlertSts
  misc/hwmon: sbrmi: Add support for APML protocols
  misc: sbrmi: Add support for EPYC platform Family: 0x1A and Model:
    0x0~0x1F

 drivers/hwmon/Kconfig            |   1 +
 drivers/hwmon/sbrmi.c            | 271 ++------------
 drivers/misc/Kconfig             |   1 +
 drivers/misc/Makefile            |   1 +
 drivers/misc/amd-sb/Kconfig      |   9 +
 drivers/misc/amd-sb/Makefile     |   3 +
 drivers/misc/amd-sb/sbrmi-core.c | 617 +++++++++++++++++++++++++++++++
 drivers/misc/amd-sb/sbrmi-core.h |  36 ++
 drivers/misc/amd-sb/sbrmi-i2c.c  | 239 ++++++++++++
 include/misc/amd-sb.h            |  57 +++
 include/uapi/linux/amd-apml.h    |  74 ++++
 11 files changed, 1072 insertions(+), 237 deletions(-)
 create mode 100644 drivers/misc/amd-sb/Kconfig
 create mode 100644 drivers/misc/amd-sb/Makefile
 create mode 100644 drivers/misc/amd-sb/sbrmi-core.c
 create mode 100644 drivers/misc/amd-sb/sbrmi-core.h
 create mode 100644 drivers/misc/amd-sb/sbrmi-i2c.c
 create mode 100644 include/misc/amd-sb.h
 create mode 100644 include/uapi/linux/amd-apml.h

-- 
2.25.1

[PATCH 1/6] hwmon/misc sbrmi: Move core sbrmi from hwmon to misc

[Akshay Gupta]

This is done to support other functionality provided by the SBRMI, which
does not fit in the hwmon subsystem.

- Move the SBRMI core functionality and I2C device probing part to misc.
- Modify hwmon/sbrmi.c sensor to be probed as a platform device.

Signed-off-by: Akshay Gupta <akshay.gupta@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
 drivers/hwmon/Kconfig            |   1 +
 drivers/hwmon/sbrmi.c            | 251 +++----------------------------
 drivers/misc/Kconfig             |   1 +
 drivers/misc/Makefile            |   1 +
 drivers/misc/amd-sb/Kconfig      |   9 ++
 drivers/misc/amd-sb/Makefile     |   3 +
 drivers/misc/amd-sb/sbrmi-core.c | 137 +++++++++++++++++
 drivers/misc/amd-sb/sbrmi-i2c.c  | 106 +++++++++++++
 include/misc/amd-sb.h            |  45 ++++++
 9 files changed, 326 insertions(+), 228 deletions(-)
 create mode 100644 drivers/misc/amd-sb/Kconfig
 create mode 100644 drivers/misc/amd-sb/Makefile
 create mode 100644 drivers/misc/amd-sb/sbrmi-core.c
 create mode 100644 drivers/misc/amd-sb/sbrmi-i2c.c
 create mode 100644 include/misc/amd-sb.h

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e14ae18a973b..1d8716815630 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1839,6 +1839,7 @@ config SENSORS_SBTSI
 config SENSORS_SBRMI
 	tristate "Emulated SB-RMI sensor"
 	depends on I2C
+	depends on AMD_SBRMI_I2C
 	help
 	  If you say yes here you get support for emulated RMI
 	  sensors on AMD SoCs with APML interface connected to a BMC device.
diff --git a/drivers/hwmon/sbrmi.c b/drivers/hwmon/sbrmi.c
index d48d8e5460ff..90b8f94a43fa 100644
--- a/drivers/hwmon/sbrmi.c
+++ b/drivers/hwmon/sbrmi.c
@@ -3,190 +3,18 @@
  * sbrmi.c - hwmon driver for a SB-RMI mailbox
  *           compliant AMD SoC device.
  *
- * Copyright (C) 2020-2021 Advanced Micro Devices, Inc.
+ * Copyright (C) 2020-2024 Advanced Micro Devices, Inc.
  */
 
-#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
-#include <linux/i2c.h>
-#include <linux/init.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
-#include <linux/of.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <misc/amd-sb.h>
 
 /* Do not allow setting negative power limit */
 #define SBRMI_PWR_MIN	0
-/* Mask for Status Register bit[1] */
-#define SW_ALERT_MASK	0x2
-
-/* Software Interrupt for triggering */
-#define START_CMD	0x80
-#define TRIGGER_MAILBOX	0x01
-
-/*
- * SB-RMI supports soft mailbox service request to MP1 (power management
- * firmware) through SBRMI inbound/outbound message registers.
- * SB-RMI message IDs
- */
-enum sbrmi_msg_id {
-	SBRMI_READ_PKG_PWR_CONSUMPTION = 0x1,
-	SBRMI_WRITE_PKG_PWR_LIMIT,
-	SBRMI_READ_PKG_PWR_LIMIT,
-	SBRMI_READ_PKG_MAX_PWR_LIMIT,
-};
-
-/* SB-RMI registers */
-enum sbrmi_reg {
-	SBRMI_CTRL		= 0x01,
-	SBRMI_STATUS,
-	SBRMI_OUTBNDMSG0	= 0x30,
-	SBRMI_OUTBNDMSG1,
-	SBRMI_OUTBNDMSG2,
-	SBRMI_OUTBNDMSG3,
-	SBRMI_OUTBNDMSG4,
-	SBRMI_OUTBNDMSG5,
-	SBRMI_OUTBNDMSG6,
-	SBRMI_OUTBNDMSG7,
-	SBRMI_INBNDMSG0,
-	SBRMI_INBNDMSG1,
-	SBRMI_INBNDMSG2,
-	SBRMI_INBNDMSG3,
-	SBRMI_INBNDMSG4,
-	SBRMI_INBNDMSG5,
-	SBRMI_INBNDMSG6,
-	SBRMI_INBNDMSG7,
-	SBRMI_SW_INTERRUPT,
-};
-
-/* Each client has this additional data */
-struct sbrmi_data {
-	struct i2c_client *client;
-	struct mutex lock;
-	u32 pwr_limit_max;
-};
-
-struct sbrmi_mailbox_msg {
-	u8 cmd;
-	bool read;
-	u32 data_in;
-	u32 data_out;
-};
-
-static int sbrmi_enable_alert(struct i2c_client *client)
-{
-	int ctrl;
-
-	/*
-	 * Enable the SB-RMI Software alert status
-	 * by writing 0 to bit 4 of Control register(0x1)
-	 */
-	ctrl = i2c_smbus_read_byte_data(client, SBRMI_CTRL);
-	if (ctrl < 0)
-		return ctrl;
-
-	if (ctrl & 0x10) {
-		ctrl &= ~0x10;
-		return i2c_smbus_write_byte_data(client,
-						 SBRMI_CTRL, ctrl);
-	}
-
-	return 0;
-}
-
-static int rmi_mailbox_xfer(struct sbrmi_data *data,
-			    struct sbrmi_mailbox_msg *msg)
-{
-	int i, ret, retry = 10;
-	int sw_status;
-	u8 byte;
-
-	mutex_lock(&data->lock);
-
-	/* Indicate firmware a command is to be serviced */
-	ret = i2c_smbus_write_byte_data(data->client,
-					SBRMI_INBNDMSG7, START_CMD);
-	if (ret < 0)
-		goto exit_unlock;
-
-	/* Write the command to SBRMI::InBndMsg_inst0 */
-	ret = i2c_smbus_write_byte_data(data->client,
-					SBRMI_INBNDMSG0, msg->cmd);
-	if (ret < 0)
-		goto exit_unlock;
-
-	/*
-	 * For both read and write the initiator (BMC) writes
-	 * Command Data In[31:0] to SBRMI::InBndMsg_inst[4:1]
-	 * SBRMI_x3C(MSB):SBRMI_x39(LSB)
-	 */
-	for (i = 0; i < 4; i++) {
-		byte = (msg->data_in >> i * 8) & 0xff;
-		ret = i2c_smbus_write_byte_data(data->client,
-						SBRMI_INBNDMSG1 + i, byte);
-		if (ret < 0)
-			goto exit_unlock;
-	}
-
-	/*
-	 * Write 0x01 to SBRMI::SoftwareInterrupt to notify firmware to
-	 * perform the requested read or write command
-	 */
-	ret = i2c_smbus_write_byte_data(data->client,
-					SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX);
-	if (ret < 0)
-		goto exit_unlock;
-
-	/*
-	 * Firmware will write SBRMI::Status[SwAlertSts]=1 to generate
-	 * an ALERT (if enabled) to initiator (BMC) to indicate completion
-	 * of the requested command
-	 */
-	do {
-		sw_status = i2c_smbus_read_byte_data(data->client,
-						     SBRMI_STATUS);
-		if (sw_status < 0) {
-			ret = sw_status;
-			goto exit_unlock;
-		}
-		if (sw_status & SW_ALERT_MASK)
-			break;
-		usleep_range(50, 100);
-	} while (retry--);
-
-	if (retry < 0) {
-		dev_err(&data->client->dev,
-			"Firmware fail to indicate command completion\n");
-		ret = -EIO;
-		goto exit_unlock;
-	}
-
-	/*
-	 * For a read operation, the initiator (BMC) reads the firmware
-	 * response Command Data Out[31:0] from SBRMI::OutBndMsg_inst[4:1]
-	 * {SBRMI_x34(MSB):SBRMI_x31(LSB)}.
-	 */
-	if (msg->read) {
-		for (i = 0; i < 4; i++) {
-			ret = i2c_smbus_read_byte_data(data->client,
-						       SBRMI_OUTBNDMSG1 + i);
-			if (ret < 0)
-				goto exit_unlock;
-			msg->data_out |= ret << i * 8;
-		}
-	}
-
-	/*
-	 * BMC must write 1'b1 to SBRMI::Status[SwAlertSts] to clear the
-	 * ALERT to initiator
-	 */
-	ret = i2c_smbus_write_byte_data(data->client, SBRMI_STATUS,
-					sw_status | SW_ALERT_MASK);
-
-exit_unlock:
-	mutex_unlock(&data->lock);
-	return ret;
-}
 
 static int sbrmi_read(struct device *dev, enum hwmon_sensor_types type,
 		      u32 attr, int channel, long *val)
@@ -195,6 +23,9 @@ static int sbrmi_read(struct device *dev, enum hwmon_sensor_types type,
 	struct sbrmi_mailbox_msg msg = { 0 };
 	int ret;
 
+	if (!data)
+		return -ENODEV;
+
 	if (type != hwmon_power)
 		return -EINVAL;
 
@@ -228,6 +59,9 @@ static int sbrmi_write(struct device *dev, enum hwmon_sensor_types type,
 	struct sbrmi_data *data = dev_get_drvdata(dev);
 	struct sbrmi_mailbox_msg msg = { 0 };
 
+	if (!data)
+		return -ENODEV;
+
 	if (type != hwmon_power && attr != hwmon_power_cap)
 		return -EINVAL;
 	/*
@@ -282,76 +116,37 @@ static const struct hwmon_chip_info sbrmi_chip_info = {
 	.info = sbrmi_info,
 };
 
-static int sbrmi_get_max_pwr_limit(struct sbrmi_data *data)
+static int sbrmi_probe(struct platform_device *pdev)
 {
-	struct sbrmi_mailbox_msg msg = { 0 };
-	int ret;
-
-	msg.cmd = SBRMI_READ_PKG_MAX_PWR_LIMIT;
-	msg.read = true;
-	ret = rmi_mailbox_xfer(data, &msg);
-	if (ret < 0)
-		return ret;
-	data->pwr_limit_max = msg.data_out;
-
-	return ret;
-}
-
-static int sbrmi_probe(struct i2c_client *client)
-{
-	struct device *dev = &client->dev;
+	struct device *dev = &pdev->dev;
 	struct device *hwmon_dev;
-	struct sbrmi_data *data;
-	int ret;
+	struct sbrmi_data *data  = dev_get_drvdata(pdev->dev.parent);
 
-	data = devm_kzalloc(dev, sizeof(struct sbrmi_data), GFP_KERNEL);
 	if (!data)
-		return -ENOMEM;
-
-	data->client = client;
-	mutex_init(&data->lock);
-
-	/* Enable alert for SB-RMI sequence */
-	ret = sbrmi_enable_alert(client);
-	if (ret < 0)
-		return ret;
-
-	/* Cache maximum power limit */
-	ret = sbrmi_get_max_pwr_limit(data);
-	if (ret < 0)
-		return ret;
-
-	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
+		return -ENODEV;
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "sbrmi", data,
 							 &sbrmi_chip_info, NULL);
-
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-static const struct i2c_device_id sbrmi_id[] = {
-	{"sbrmi"},
-	{}
-};
-MODULE_DEVICE_TABLE(i2c, sbrmi_id);
-
-static const struct of_device_id __maybe_unused sbrmi_of_match[] = {
+static const struct platform_device_id sbrmi_id[] = {
 	{
-		.compatible = "amd,sbrmi",
+		.name = "sbrmi-hwmon",
 	},
 	{ },
 };
-MODULE_DEVICE_TABLE(of, sbrmi_of_match);
+MODULE_DEVICE_TABLE(platform, sbrmi_id);
 
-static struct i2c_driver sbrmi_driver = {
+static struct platform_driver amd_sbrmi_hwmon_driver = {
+	.probe = sbrmi_probe,
 	.driver = {
-		.name = "sbrmi",
-		.of_match_table = of_match_ptr(sbrmi_of_match),
+		.name = "amd-sbrmi",
 	},
-	.probe = sbrmi_probe,
 	.id_table = sbrmi_id,
 };
-
-module_i2c_driver(sbrmi_driver);
+module_platform_driver(amd_sbrmi_hwmon_driver);
 
 MODULE_AUTHOR("Akshay Gupta <akshay.gupta@amd.com>");
+MODULE_AUTHOR("Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>");
 MODULE_DESCRIPTION("Hwmon driver for AMD SB-RMI emulated sensor");
 MODULE_LICENSE("GPL");
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index faf983680040..48df5757af96 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -602,4 +602,5 @@ source "drivers/misc/cardreader/Kconfig"
 source "drivers/misc/uacce/Kconfig"
 source "drivers/misc/pvpanic/Kconfig"
 source "drivers/misc/mchp_pci1xxxx/Kconfig"
+source "drivers/misc/amd-sb/Kconfig"
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 153a3f4837e8..561e0d635b4f 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -69,3 +69,4 @@ obj-$(CONFIG_TMR_INJECT)	+= xilinx_tmr_inject.o
 obj-$(CONFIG_TPS6594_ESM)	+= tps6594-esm.o
 obj-$(CONFIG_TPS6594_PFSM)	+= tps6594-pfsm.o
 obj-$(CONFIG_NSM)		+= nsm.o
+obj-y				+= amd-sb/
diff --git a/drivers/misc/amd-sb/Kconfig b/drivers/misc/amd-sb/Kconfig
new file mode 100644
index 000000000000..d47bdd26a871
--- /dev/null
+++ b/drivers/misc/amd-sb/Kconfig
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config AMD_SBRMI_I2C
+        tristate "APML SBRMI support"
+        depends on I2C
+        help
+          APML RMI core support for AMD out of band management.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called apml-sbrmi.
diff --git a/drivers/misc/amd-sb/Makefile b/drivers/misc/amd-sb/Makefile
new file mode 100644
index 000000000000..0f9277e7335a
--- /dev/null
+++ b/drivers/misc/amd-sb/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+apml-sbrmi-objs  		:= sbrmi-i2c.o sbrmi-core.o
+obj-$(CONFIG_AMD_SBRMI_I2C)	+= apml-sbrmi.o
diff --git a/drivers/misc/amd-sb/sbrmi-core.c b/drivers/misc/amd-sb/sbrmi-core.c
new file mode 100644
index 000000000000..5ca0a9ec8341
--- /dev/null
+++ b/drivers/misc/amd-sb/sbrmi-core.c
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * sbrmi-common.c - file defining SB-RMI protocols
+ *		    compliant AMD SoC device.
+ *
+ * Copyright (C) 2021-2024 Advanced Micro Devices, Inc.
+ */
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <misc/amd-sb.h>
+
+/* Mask for Status Register bit[1] */
+#define SW_ALERT_MASK	0x2
+
+/* Software Interrupt for triggering */
+#define START_CMD	0x80
+#define TRIGGER_MAILBOX	0x01
+
+/* SB-RMI registers */
+enum sbrmi_reg {
+	SBRMI_CTRL		= 0x01,
+	SBRMI_STATUS,
+	SBRMI_OUTBNDMSG0	= 0x30,
+	SBRMI_OUTBNDMSG1,
+	SBRMI_OUTBNDMSG2,
+	SBRMI_OUTBNDMSG3,
+	SBRMI_OUTBNDMSG4,
+	SBRMI_OUTBNDMSG5,
+	SBRMI_OUTBNDMSG6,
+	SBRMI_OUTBNDMSG7,
+	SBRMI_INBNDMSG0,
+	SBRMI_INBNDMSG1,
+	SBRMI_INBNDMSG2,
+	SBRMI_INBNDMSG3,
+	SBRMI_INBNDMSG4,
+	SBRMI_INBNDMSG5,
+	SBRMI_INBNDMSG6,
+	SBRMI_INBNDMSG7,
+	SBRMI_SW_INTERRUPT,
+};
+
+int rmi_mailbox_xfer(struct sbrmi_data *data,
+		     struct sbrmi_mailbox_msg *msg)
+{
+	int i, ret, retry = 10;
+	int sw_status;
+	u8 byte;
+
+	mutex_lock(&data->lock);
+
+	/* Indicate firmware a command is to be serviced */
+	ret = i2c_smbus_write_byte_data(data->client,
+					SBRMI_INBNDMSG7, START_CMD);
+	if (ret < 0)
+		goto exit_unlock;
+
+	/* Write the command to SBRMI::InBndMsg_inst0 */
+	ret = i2c_smbus_write_byte_data(data->client,
+					SBRMI_INBNDMSG0, msg->cmd);
+	if (ret < 0)
+		goto exit_unlock;
+
+	/*
+	 * For both read and write the initiator (BMC) writes
+	 * Command Data In[31:0] to SBRMI::InBndMsg_inst[4:1]
+	 * SBRMI_x3C(MSB):SBRMI_x39(LSB)
+	 */
+	for (i = 0; i < 4; i++) {
+		byte = (msg->data_in >> i * 8) & 0xff;
+		ret = i2c_smbus_write_byte_data(data->client,
+						SBRMI_INBNDMSG1 + i, byte);
+		if (ret < 0)
+			goto exit_unlock;
+	}
+
+	/*
+	 * Write 0x01 to SBRMI::SoftwareInterrupt to notify firmware to
+	 * perform the requested read or write command
+	 */
+	ret = i2c_smbus_write_byte_data(data->client,
+					SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX);
+	if (ret < 0)
+		goto exit_unlock;
+
+	/*
+	 * Firmware will write SBRMI::Status[SwAlertSts]=1 to generate
+	 * an ALERT (if enabled) to initiator (BMC) to indicate completion
+	 * of the requested command
+	 */
+	do {
+		sw_status = i2c_smbus_read_byte_data(data->client,
+						     SBRMI_STATUS);
+		if (sw_status < 0) {
+			ret = sw_status;
+			goto exit_unlock;
+		}
+		if (sw_status & SW_ALERT_MASK)
+			break;
+		usleep_range(50, 100);
+	} while (retry--);
+
+	if (retry < 0) {
+		dev_err(&data->client->dev,
+			"Firmware fail to indicate command completion\n");
+		ret = -EIO;
+		goto exit_unlock;
+	}
+
+	/*
+	 * For a read operation, the initiator (BMC) reads the firmware
+	 * response Command Data Out[31:0] from SBRMI::OutBndMsg_inst[4:1]
+	 * {SBRMI_x34(MSB):SBRMI_x31(LSB)}.
+	 */
+	if (msg->read) {
+		for (i = 0; i < 4; i++) {
+			ret = i2c_smbus_read_byte_data(data->client,
+						       SBRMI_OUTBNDMSG1 + i);
+			if (ret < 0)
+				goto exit_unlock;
+			msg->data_out |= ret << i * 8;
+		}
+	}
+
+	/*
+	 * BMC must write 1'b1 to SBRMI::Status[SwAlertSts] to clear the
+	 * ALERT to initiator
+	 */
+	ret = i2c_smbus_write_byte_data(data->client, SBRMI_STATUS,
+					sw_status | SW_ALERT_MASK);
+
+exit_unlock:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+EXPORT_SYMBOL(rmi_mailbox_xfer);
diff --git a/drivers/misc/amd-sb/sbrmi-i2c.c b/drivers/misc/amd-sb/sbrmi-i2c.c
new file mode 100644
index 000000000000..c4903d9e9f0f
--- /dev/null
+++ b/drivers/misc/amd-sb/sbrmi-i2c.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * sbrmi.c - hwmon driver for a SB-RMI mailbox
+ *           compliant AMD SoC device.
+ *
+ * Copyright (C) 2020-2024 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <misc/amd-sb.h>
+
+#define SBRMI_CTRL	0x1
+
+static int sbrmi_enable_alert(struct i2c_client *client)
+{
+	int ctrl;
+
+	/*
+	 * Enable the SB-RMI Software alert status
+	 * by writing 0 to bit 4 of Control register(0x1)
+	 */
+	ctrl = i2c_smbus_read_byte_data(client, SBRMI_CTRL);
+	if (ctrl < 0)
+		return ctrl;
+
+	if (ctrl & 0x10) {
+		ctrl &= ~0x10;
+		return i2c_smbus_write_byte_data(client,
+						 SBRMI_CTRL, ctrl);
+	}
+
+	return 0;
+}
+
+static int sbrmi_get_max_pwr_limit(struct sbrmi_data *data)
+{
+	struct sbrmi_mailbox_msg msg = { 0 };
+	int ret;
+
+	msg.cmd = SBRMI_READ_PKG_MAX_PWR_LIMIT;
+	msg.read = true;
+	ret = rmi_mailbox_xfer(data, &msg);
+	if (ret < 0)
+		return ret;
+	data->pwr_limit_max = msg.data_out;
+
+	return ret;
+}
+
+static int sbrmi_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct sbrmi_data *data;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(struct sbrmi_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	mutex_init(&data->lock);
+
+	/* Enable alert for SB-RMI sequence */
+	ret = sbrmi_enable_alert(client);
+	if (ret < 0)
+		return ret;
+
+	/* Cache maximum power limit */
+	return sbrmi_get_max_pwr_limit(data);
+}
+
+static const struct i2c_device_id sbrmi_id[] = {
+	{"sbrmi-i2c"},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, sbrmi_id);
+
+static const struct of_device_id __maybe_unused sbrmi_of_match[] = {
+	{
+		.compatible = "amd,sbrmi",
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sbrmi_of_match);
+
+static struct i2c_driver sbrmi_driver = {
+	.driver = {
+		.name = "sbrmi-i2c",
+		.of_match_table = of_match_ptr(sbrmi_of_match),
+	},
+	.probe = sbrmi_i2c_probe,
+	.id_table = sbrmi_id,
+};
+
+module_i2c_driver(sbrmi_driver);
+
+MODULE_AUTHOR("Akshay Gupta <akshay.gupta@amd.com>");
+MODULE_AUTHOR("Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>");
+MODULE_DESCRIPTION("Hwmon driver for AMD SB-RMI emulated sensor");
+MODULE_LICENSE("GPL");
diff --git a/include/misc/amd-sb.h b/include/misc/amd-sb.h
new file mode 100644
index 000000000000..e1a012fcdff9
--- /dev/null
+++ b/include/misc/amd-sb.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2021-2024 Advanced Micro Devices, Inc.
+ */
+
+#ifndef _AMD_SB_H_
+#define _AMD_SB_H_
+
+#include <linux/mutex.h>
+#include <linux/i2c.h>
+/*
+ * SB-RMI supports soft mailbox service request to MP1 (power management
+ * firmware) through SBRMI inbound/outbound message registers.
+ * SB-RMI message IDs
+ */
+enum sbrmi_msg_id {
+	SBRMI_READ_PKG_PWR_CONSUMPTION = 0x1,
+	SBRMI_WRITE_PKG_PWR_LIMIT,
+	SBRMI_READ_PKG_PWR_LIMIT,
+	SBRMI_READ_PKG_MAX_PWR_LIMIT,
+};
+
+/* Each client has this additional data */
+struct sbrmi_data {
+	struct i2c_client *client;
+	struct mutex lock;
+	u32 pwr_limit_max;
+};
+
+struct sbrmi_mailbox_msg {
+	u8 cmd;
+	bool read;
+	u32 data_in;
+	u32 data_out;
+};
+
+#if IS_ENABLED(CONFIG_AMD_SBRMI_I2C)
+int rmi_mailbox_xfer(struct sbrmi_data *data, struct sbrmi_mailbox_msg *msg);
+#else
+int rmi_mailbox_xfer(struct sbrmi_data *data, struct sbrmi_mailbox_msg *msg)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+#endif /*_AMD_SB_H_*/
-- 
2.25.1

[PATCH 2/6] misc: sbrmi: Add platform device add to create platform device

[Akshay Gupta]

- AMD provides socket power information from out of band
  which can be read by sensors.
- platform driver will probe drivers/hwmon/sbrmi as a platform device
  and share the sbrmi device data.

Signed-off-by: Akshay Gupta <akshay.gupta@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
 drivers/misc/amd-sb/sbrmi-i2c.c | 25 ++++++++++++++++++++++++-
 include/misc/amd-sb.h           |  2 ++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/amd-sb/sbrmi-i2c.c b/drivers/misc/amd-sb/sbrmi-i2c.c
index c4903d9e9f0f..b593bbdd78e0 100644
--- a/drivers/misc/amd-sb/sbrmi-i2c.c
+++ b/drivers/misc/amd-sb/sbrmi-i2c.c
@@ -72,7 +72,29 @@ static int sbrmi_i2c_probe(struct i2c_client *client)
 		return ret;
 
 	/* Cache maximum power limit */
-	return sbrmi_get_max_pwr_limit(data);
+	ret = sbrmi_get_max_pwr_limit(data);
+	if (ret < 0)
+		return ret;
+
+	dev_set_drvdata(dev, (void *)data);
+	data->pdev = platform_device_register_data(dev, "sbrmi-hwmon",
+						   PLATFORM_DEVID_NONE,
+						   data,
+						   sizeof(struct sbrmi_data));
+	if (IS_ERR(data->pdev)) {
+		pr_err("unable to register platform device for sbrmi-hwmon\n");
+		return PTR_ERR(data->pdev);
+	}
+	return ret;
+}
+
+static void sbrmi_i2c_remove(struct i2c_client *client)
+{
+	struct sbrmi_data *data = dev_get_drvdata(&client->dev);
+
+	if (!data)
+		return;
+	platform_device_unregister(data->pdev);
 }
 
 static const struct i2c_device_id sbrmi_id[] = {
@@ -95,6 +117,7 @@ static struct i2c_driver sbrmi_driver = {
 		.of_match_table = of_match_ptr(sbrmi_of_match),
 	},
 	.probe = sbrmi_i2c_probe,
+	.remove = sbrmi_i2c_remove,
 	.id_table = sbrmi_id,
 };
 
diff --git a/include/misc/amd-sb.h b/include/misc/amd-sb.h
index e1a012fcdff9..79b76dd6068a 100644
--- a/include/misc/amd-sb.h
+++ b/include/misc/amd-sb.h
@@ -8,6 +8,7 @@
 
 #include <linux/mutex.h>
 #include <linux/i2c.h>
+#include <linux/platform_device.h>
 /*
  * SB-RMI supports soft mailbox service request to MP1 (power management
  * firmware) through SBRMI inbound/outbound message registers.
@@ -24,6 +25,7 @@ enum sbrmi_msg_id {
 struct sbrmi_data {
 	struct i2c_client *client;
 	struct mutex lock;
+	struct platform_device *pdev;
 	u32 pwr_limit_max;
 };
 
-- 
2.25.1

[PATCH 3/6] misc: sbrmi: Use regmap subsystem

[Akshay Gupta]

- regmap subsystem provides multiple benefits over direct smbus APIs
- The susbsytem can be helpful in following cases
  - Differnet types of bus (i2c/i3c)
  - Different Register address size (1byte/2byte)

Signed-off-by: Akshay Gupta <akshay.gupta@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
 drivers/misc/amd-sb/sbrmi-core.c | 29 ++++++++++++-----------------
 drivers/misc/amd-sb/sbrmi-i2c.c  | 25 ++++++++++++++++---------
 include/misc/amd-sb.h            |  6 +++---
 3 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/misc/amd-sb/sbrmi-core.c b/drivers/misc/amd-sb/sbrmi-core.c
index 5ca0a9ec8341..b2437a4044ac 100644
--- a/drivers/misc/amd-sb/sbrmi-core.c
+++ b/drivers/misc/amd-sb/sbrmi-core.c
@@ -9,6 +9,7 @@
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/mutex.h>
+#include <linux/regmap.h>
 #include <misc/amd-sb.h>
 
 /* Mask for Status Register bit[1] */
@@ -44,6 +45,7 @@ enum sbrmi_reg {
 int rmi_mailbox_xfer(struct sbrmi_data *data,
 		     struct sbrmi_mailbox_msg *msg)
 {
+	unsigned int bytes;
 	int i, ret, retry = 10;
 	int sw_status;
 	u8 byte;
@@ -51,14 +53,12 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	mutex_lock(&data->lock);
 
 	/* Indicate firmware a command is to be serviced */
-	ret = i2c_smbus_write_byte_data(data->client,
-					SBRMI_INBNDMSG7, START_CMD);
+	ret = regmap_write(data->regmap, SBRMI_INBNDMSG7, START_CMD);
 	if (ret < 0)
 		goto exit_unlock;
 
 	/* Write the command to SBRMI::InBndMsg_inst0 */
-	ret = i2c_smbus_write_byte_data(data->client,
-					SBRMI_INBNDMSG0, msg->cmd);
+	ret = regmap_write(data->regmap, SBRMI_INBNDMSG0, msg->cmd);
 	if (ret < 0)
 		goto exit_unlock;
 
@@ -69,8 +69,7 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	 */
 	for (i = 0; i < 4; i++) {
 		byte = (msg->data_in >> i * 8) & 0xff;
-		ret = i2c_smbus_write_byte_data(data->client,
-						SBRMI_INBNDMSG1 + i, byte);
+		ret = regmap_write(data->regmap, SBRMI_INBNDMSG1 + i, byte);
 		if (ret < 0)
 			goto exit_unlock;
 	}
@@ -79,8 +78,7 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	 * Write 0x01 to SBRMI::SoftwareInterrupt to notify firmware to
 	 * perform the requested read or write command
 	 */
-	ret = i2c_smbus_write_byte_data(data->client,
-					SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX);
+	ret = regmap_write(data->regmap, SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX);
 	if (ret < 0)
 		goto exit_unlock;
 
@@ -90,8 +88,7 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	 * of the requested command
 	 */
 	do {
-		sw_status = i2c_smbus_read_byte_data(data->client,
-						     SBRMI_STATUS);
+		ret = regmap_read(data->regmap, SBRMI_STATUS, &sw_status);
 		if (sw_status < 0) {
 			ret = sw_status;
 			goto exit_unlock;
@@ -102,8 +99,6 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	} while (retry--);
 
 	if (retry < 0) {
-		dev_err(&data->client->dev,
-			"Firmware fail to indicate command completion\n");
 		ret = -EIO;
 		goto exit_unlock;
 	}
@@ -115,11 +110,11 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	 */
 	if (msg->read) {
 		for (i = 0; i < 4; i++) {
-			ret = i2c_smbus_read_byte_data(data->client,
-						       SBRMI_OUTBNDMSG1 + i);
+			ret = regmap_read(data->regmap,
+					  SBRMI_OUTBNDMSG1 + i, &bytes);
 			if (ret < 0)
 				goto exit_unlock;
-			msg->data_out |= ret << i * 8;
+			msg->data_out |= bytes << i * 8;
 		}
 	}
 
@@ -127,8 +122,8 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	 * BMC must write 1'b1 to SBRMI::Status[SwAlertSts] to clear the
 	 * ALERT to initiator
 	 */
-	ret = i2c_smbus_write_byte_data(data->client, SBRMI_STATUS,
-					sw_status | SW_ALERT_MASK);
+	ret = regmap_write(data->regmap, SBRMI_STATUS,
+			   sw_status | SW_ALERT_MASK);
 
 exit_unlock:
 	mutex_unlock(&data->lock);
diff --git a/drivers/misc/amd-sb/sbrmi-i2c.c b/drivers/misc/amd-sb/sbrmi-i2c.c
index b593bbdd78e0..539539c42754 100644
--- a/drivers/misc/amd-sb/sbrmi-i2c.c
+++ b/drivers/misc/amd-sb/sbrmi-i2c.c
@@ -13,26 +13,26 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/regmap.h>
 #include <misc/amd-sb.h>
 
 #define SBRMI_CTRL	0x1
 
-static int sbrmi_enable_alert(struct i2c_client *client)
+static int sbrmi_enable_alert(struct sbrmi_data *data)
 {
-	int ctrl;
+	int ctrl, ret;
 
 	/*
 	 * Enable the SB-RMI Software alert status
 	 * by writing 0 to bit 4 of Control register(0x1)
 	 */
-	ctrl = i2c_smbus_read_byte_data(client, SBRMI_CTRL);
-	if (ctrl < 0)
-		return ctrl;
+	ret = regmap_read(data->regmap, SBRMI_CTRL, &ctrl);
+	if (ret < 0)
+		return ret;
 
 	if (ctrl & 0x10) {
 		ctrl &= ~0x10;
-		return i2c_smbus_write_byte_data(client,
-						 SBRMI_CTRL, ctrl);
+		return regmap_write(data->regmap, SBRMI_CTRL, ctrl);
 	}
 
 	return 0;
@@ -57,17 +57,24 @@ static int sbrmi_i2c_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct sbrmi_data *data;
+	struct regmap_config sbrmi_i2c_regmap_config = {
+		.reg_bits = 8,
+		.val_bits = 8,
+	};
 	int ret;
 
 	data = devm_kzalloc(dev, sizeof(struct sbrmi_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	data->client = client;
 	mutex_init(&data->lock);
 
+	data->regmap = devm_regmap_init_i2c(client, &sbrmi_i2c_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
 	/* Enable alert for SB-RMI sequence */
-	ret = sbrmi_enable_alert(client);
+	ret = sbrmi_enable_alert(data);
 	if (ret < 0)
 		return ret;
 
diff --git a/include/misc/amd-sb.h b/include/misc/amd-sb.h
index 79b76dd6068a..71abb3035f48 100644
--- a/include/misc/amd-sb.h
+++ b/include/misc/amd-sb.h
@@ -7,8 +7,8 @@
 #define _AMD_SB_H_
 
 #include <linux/mutex.h>
-#include <linux/i2c.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 /*
  * SB-RMI supports soft mailbox service request to MP1 (power management
  * firmware) through SBRMI inbound/outbound message registers.
@@ -23,11 +23,11 @@ enum sbrmi_msg_id {
 
 /* Each client has this additional data */
 struct sbrmi_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 	struct mutex lock;
 	struct platform_device *pdev;
 	u32 pwr_limit_max;
-};
+} __packed;
 
 struct sbrmi_mailbox_msg {
 	u8 cmd;
-- 
2.25.1

[PATCH 4/6] misc: sbrmi: Clear sbrmi status register bit SwAlertSts

[Akshay Gupta]

- SBRMI register status b1' SwAlertSts sets in below conditions
  - Completion of Mailbox command
  - syncflood event
If the bit is set for syncfllod event, it may be interpretted as
mailbox command compleition. This patch clears the bit before
start of mailbox protocol.

Signed-off-by: Akshay Gupta <akshay.gupta@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
 drivers/misc/amd-sb/sbrmi-core.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/misc/amd-sb/sbrmi-core.c b/drivers/misc/amd-sb/sbrmi-core.c
index b2437a4044ac..e528b7314447 100644
--- a/drivers/misc/amd-sb/sbrmi-core.c
+++ b/drivers/misc/amd-sb/sbrmi-core.c
@@ -42,6 +42,22 @@ enum sbrmi_reg {
 	SBRMI_SW_INTERRUPT,
 };
 
+static int sbrmi_clear_status_alert(struct sbrmi_data *data)
+{
+	int sw_status, ret;
+
+	ret = regmap_read(data->regmap, SBRMI_STATUS,
+			  &sw_status);
+	if (ret < 0)
+		return ret;
+
+	if (!(sw_status & SW_ALERT_MASK))
+		return 0;
+
+	return regmap_write(data->regmap, SBRMI_STATUS,
+			    SW_ALERT_MASK);
+}
+
 int rmi_mailbox_xfer(struct sbrmi_data *data,
 		     struct sbrmi_mailbox_msg *msg)
 {
@@ -52,6 +68,10 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 
 	mutex_lock(&data->lock);
 
+	ret = sbrmi_clear_status_alert(data);
+	if (ret < 0)
+		goto exit_unlock;
+
 	/* Indicate firmware a command is to be serviced */
 	ret = regmap_write(data->regmap, SBRMI_INBNDMSG7, START_CMD);
 	if (ret < 0)
-- 
2.25.1

[PATCH 5/6] misc/hwmon: sbrmi: Add support for APML protocols

[Akshay Gupta]

The present sbrmi module only support reporting power via hwmon.
However, AMD data center range of processors support various
system management functionality Out-of-band over
Advanced Platform Management Link (APML).

Register a miscdevice, which creates a device /dev/sbrmiX with an IOCTL
interface for the user space to invoke the following protocols.
  - Mailbox read/write (already defined in sbrmi_mailbox_xfer())
  - CPUID read
  - MCAMSR read

Open-sourced and widely used https://github.com/amd/esmi_oob_library
will continue to provide user-space programmable API.

Signed-off-by: Akshay Gupta <akshay.gupta@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
 drivers/hwmon/sbrmi.c            |  22 +-
 drivers/misc/amd-sb/sbrmi-core.c | 462 +++++++++++++++++++++++++++----
 drivers/misc/amd-sb/sbrmi-core.h |  36 +++
 drivers/misc/amd-sb/sbrmi-i2c.c  |  39 ++-
 include/misc/amd-sb.h            |  30 +-
 include/uapi/linux/amd-apml.h    |  74 +++++
 6 files changed, 589 insertions(+), 74 deletions(-)
 create mode 100644 drivers/misc/amd-sb/sbrmi-core.h
 create mode 100644 include/uapi/linux/amd-apml.h

diff --git a/drivers/hwmon/sbrmi.c b/drivers/hwmon/sbrmi.c
index 90b8f94a43fa..564a5a2a418d 100644
--- a/drivers/hwmon/sbrmi.c
+++ b/drivers/hwmon/sbrmi.c
@@ -20,7 +20,7 @@ static int sbrmi_read(struct device *dev, enum hwmon_sensor_types type,
 		      u32 attr, int channel, long *val)
 {
 	struct sbrmi_data *data = dev_get_drvdata(dev);
-	struct sbrmi_mailbox_msg msg = { 0 };
+	struct apml_message msg = { 0 };
 	int ret;
 
 	if (!data)
@@ -29,7 +29,7 @@ static int sbrmi_read(struct device *dev, enum hwmon_sensor_types type,
 	if (type != hwmon_power)
 		return -EINVAL;
 
-	msg.read = true;
+	msg.data_in.reg_in[RD_FLAG_INDEX] = 1;
 	switch (attr) {
 	case hwmon_power_input:
 		msg.cmd = SBRMI_READ_PKG_PWR_CONSUMPTION;
@@ -40,16 +40,17 @@ static int sbrmi_read(struct device *dev, enum hwmon_sensor_types type,
 		ret = rmi_mailbox_xfer(data, &msg);
 		break;
 	case hwmon_power_cap_max:
-		msg.data_out = data->pwr_limit_max;
+		msg.data_out.mb_out[RD_WR_DATA_INDEX] = data->pwr_limit_max;
 		ret = 0;
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
-	if (ret < 0)
-		return ret;
+
 	/* hwmon power attributes are in microWatt */
-	*val = (long)msg.data_out * 1000;
+	if (!ret)
+		*val = (long)msg.data_out.mb_out[RD_WR_DATA_INDEX] * 1000;
+
 	return ret;
 }
 
@@ -57,7 +58,7 @@ static int sbrmi_write(struct device *dev, enum hwmon_sensor_types type,
 		       u32 attr, int channel, long val)
 {
 	struct sbrmi_data *data = dev_get_drvdata(dev);
-	struct sbrmi_mailbox_msg msg = { 0 };
+	struct apml_message msg = { 0 };
 
 	if (!data)
 		return -ENODEV;
@@ -73,8 +74,8 @@ static int sbrmi_write(struct device *dev, enum hwmon_sensor_types type,
 	val = clamp_val(val, SBRMI_PWR_MIN, data->pwr_limit_max);
 
 	msg.cmd = SBRMI_WRITE_PKG_PWR_LIMIT;
-	msg.data_in = val;
-	msg.read = false;
+	msg.data_in.mb_in[RD_WR_DATA_INDEX] = val;
+	msg.data_in.reg_in[RD_FLAG_INDEX] = 0;
 
 	return rmi_mailbox_xfer(data, &msg);
 }
@@ -124,6 +125,7 @@ static int sbrmi_probe(struct platform_device *pdev)
 
 	if (!data)
 		return -ENODEV;
+
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, "sbrmi", data,
 							 &sbrmi_chip_info, NULL);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
diff --git a/drivers/misc/amd-sb/sbrmi-core.c b/drivers/misc/amd-sb/sbrmi-core.c
index e528b7314447..c180f58b92c3 100644
--- a/drivers/misc/amd-sb/sbrmi-core.c
+++ b/drivers/misc/amd-sb/sbrmi-core.c
@@ -7,40 +7,266 @@
  */
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/fs.h>
 #include <linux/i2c.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/regmap.h>
 #include <misc/amd-sb.h>
+#include "sbrmi-core.h"
 
 /* Mask for Status Register bit[1] */
 #define SW_ALERT_MASK	0x2
+/* Mask to check H/W Alert status bit */
+#define HW_ALERT_MASK	0x80
 
 /* Software Interrupt for triggering */
 #define START_CMD	0x80
 #define TRIGGER_MAILBOX	0x01
 
-/* SB-RMI registers */
-enum sbrmi_reg {
-	SBRMI_CTRL		= 0x01,
-	SBRMI_STATUS,
-	SBRMI_OUTBNDMSG0	= 0x30,
-	SBRMI_OUTBNDMSG1,
-	SBRMI_OUTBNDMSG2,
-	SBRMI_OUTBNDMSG3,
-	SBRMI_OUTBNDMSG4,
-	SBRMI_OUTBNDMSG5,
-	SBRMI_OUTBNDMSG6,
-	SBRMI_OUTBNDMSG7,
-	SBRMI_INBNDMSG0,
-	SBRMI_INBNDMSG1,
-	SBRMI_INBNDMSG2,
-	SBRMI_INBNDMSG3,
-	SBRMI_INBNDMSG4,
-	SBRMI_INBNDMSG5,
-	SBRMI_INBNDMSG6,
-	SBRMI_INBNDMSG7,
-	SBRMI_SW_INTERRUPT,
-};
+/* Default message lengths as per APML command protocol */
+/* MSR */
+#define MSR_RD_REG_LEN		0xa
+#define MSR_WR_REG_LEN		0x8
+#define MSR_RD_DATA_LEN		0x8
+#define MSR_WR_DATA_LEN		0x7
+/* CPUID */
+#define CPUID_RD_DATA_LEN	0x8
+#define CPUID_WR_DATA_LEN	0x8
+#define CPUID_RD_REG_LEN	0xa
+#define CPUID_WR_REG_LEN	0x9
+
+/* CPUID MSR Command Ids */
+#define CPUID_MCA_CMD	0x73
+#define RD_CPUID_CMD	0x91
+#define RD_MCA_CMD	0x86
+
+/* input for bulk write to CPUID and MSR protocol */
+struct cpu_msr_indata {
+	u8 wr_len;	/* const value */
+	u8 rd_len;	/* const value */
+	u8 proto_cmd;	/* const value */
+	u8 thread;	/* thread number */
+	union {
+		u8 reg_offset[4];	/* input value */
+		u32 value;
+	};
+	u8 ext; /* extended function */
+} __packed;
+
+/* output for bulk read from CPUID and MSR protocol */
+struct cpu_msr_outdata {
+	u8 num_bytes;	/* number of bytes return */
+	u8 status;	/* Protocol status code */
+	union {
+		u64 value;
+		u8 reg_data[8];
+	};
+} __packed;
+
+#define prepare_mca_msr_input_message(input, thread_id, data_in)	\
+	input.rd_len = MSR_RD_DATA_LEN,					\
+	input.wr_len = MSR_WR_DATA_LEN,					\
+	input.proto_cmd = RD_MCA_CMD,					\
+	input.thread = thread_id << 1,					\
+	input.value =  data_in
+
+#define prepare_cpuid_input_message(input, thread_id, func, ext_func)	\
+	input.rd_len = CPUID_RD_DATA_LEN,				\
+	input.wr_len = CPUID_WR_DATA_LEN,				\
+	input.proto_cmd = RD_CPUID_CMD,					\
+	input.thread = thread_id << 1,					\
+	input.value =  func,						\
+	input.ext =  ext_func
+
+static int sbrmi_get_rev(struct sbrmi_data *data)
+{
+	struct apml_message msg = { 0 };
+	int ret;
+
+	msg.data_in.reg_in[REG_OFF_INDEX] = SBRMI_REV;
+	msg.data_in.reg_in[RD_FLAG_INDEX] = 1;
+	ret = regmap_read(data->regmap,
+			  msg.data_in.reg_in[REG_OFF_INDEX],
+			  &msg.data_out.mb_out[RD_WR_DATA_INDEX]);
+	if (ret < 0)
+		return ret;
+
+	data->rev = msg.data_out.reg_out[RD_WR_DATA_INDEX];
+	return 0;
+}
+
+/*
+ * For Mailbox command software alert status bit is set by firmware
+ * to indicate command completion
+ * For RMI Rev 0x20, new h/w status bit is introduced. which is used
+ * by firmware to indicate completion of commands (0x71, 0x72, 0x73).
+ * wait for the status bit to be set by the firmware before
+ * reading the data out.
+ */
+static int sbrmi_wait_status(struct sbrmi_data *data,
+			     int *status, int mask)
+{
+	int ret, retry = 100;
+
+	do {
+		ret = regmap_read(data->regmap, SBRMI_STATUS, status);
+		if (ret < 0)
+			return ret;
+
+		if (*status & mask)
+			break;
+
+		/* Wait 1~2 second for firmware to return data out */
+		if (retry > 95)
+			usleep_range(50, 100);
+		else
+			usleep_range(10000, 20000);
+	} while (retry--);
+
+	if (retry < 0)
+		ret = -ETIMEDOUT;
+	return ret;
+}
+
+/* MCA MSR protocol */
+static int rmi_mca_msr_read(struct sbrmi_data *data,
+			    struct apml_message *msg)
+{
+	struct cpu_msr_outdata output = {0};
+	struct cpu_msr_indata input = {0};
+	int ret, val = 0;
+	int hw_status;
+	u16 thread;
+
+	/* cache the rev value to identify if protocol is supported or not */
+	if (!data->rev) {
+		ret = sbrmi_get_rev(data);
+		if (ret < 0)
+			return ret;
+	}
+	/* MCA MSR protocol for REV 0x10 is not supported*/
+	if (data->rev == 0x10)
+		return -EOPNOTSUPP;
+
+	thread = msg->data_in.reg_in[THREAD_LOW_INDEX] |
+		 msg->data_in.reg_in[THREAD_HI_INDEX] << 8;
+
+	/* Thread > 127, Thread128 CS register, 1'b1 needs to be set to 1 */
+	if (thread > 127) {
+		thread -= 128;
+		val = 1;
+	}
+	ret = regmap_write(data->regmap, SBRMI_THREAD128CS, val);
+	if (ret < 0)
+		goto exit_unlock;
+
+	prepare_mca_msr_input_message(input, thread,
+				      msg->data_in.mb_in[RD_WR_DATA_INDEX]);
+
+	ret = regmap_bulk_write(data->regmap, CPUID_MCA_CMD,
+				&input, MSR_WR_REG_LEN);
+	if (ret < 0)
+		goto exit_unlock;
+
+	ret = sbrmi_wait_status(data, &hw_status, HW_ALERT_MASK);
+	if (ret < 0)
+		goto exit_unlock;
+
+	ret = regmap_bulk_read(data->regmap, CPUID_MCA_CMD,
+			       &output, MSR_RD_REG_LEN);
+	if (ret < 0)
+		goto exit_unlock;
+
+	ret = regmap_write(data->regmap, SBRMI_STATUS,
+			   HW_ALERT_MASK);
+	if (ret < 0)
+		goto exit_unlock;
+
+	if (output.num_bytes != MSR_RD_REG_LEN - 1) {
+		ret = -EMSGSIZE;
+		goto exit_unlock;
+	}
+	if (output.status) {
+		ret = -EPROTOTYPE;
+		msg->fw_ret_code = output.status;
+		goto exit_unlock;
+	}
+	msg->data_out.cpu_msr_out = output.value;
+
+exit_unlock:
+	return ret;
+}
+
+/* Read CPUID function protocol */
+static int rmi_cpuid_read(struct sbrmi_data *data,
+			  struct apml_message *msg)
+{
+	struct cpu_msr_indata input = {0};
+	struct cpu_msr_outdata output = {0};
+	int val = 0;
+	int ret, hw_status;
+	u16 thread;
+
+	/* cache the rev value to identify if protocol is supported or not */
+	if (!data->rev) {
+		ret = sbrmi_get_rev(data);
+		if (ret < 0)
+			return ret;
+	}
+	/* CPUID protocol for REV 0x10 is not supported*/
+	if (data->rev == 0x10)
+		return -EOPNOTSUPP;
+
+	thread = msg->data_in.reg_in[THREAD_LOW_INDEX] |
+		 msg->data_in.reg_in[THREAD_HI_INDEX] << 8;
+
+	/* Thread > 127, Thread128 CS register, 1'b1 needs to be set to 1 */
+	if (thread > 127) {
+		thread -= 128;
+		val = 1;
+	}
+	ret = regmap_write(data->regmap, SBRMI_THREAD128CS, val);
+	if (ret < 0)
+		goto exit_unlock;
+
+	prepare_cpuid_input_message(input, thread,
+				    msg->data_in.mb_in[RD_WR_DATA_INDEX],
+				    msg->data_in.reg_in[EXT_FUNC_INDEX]);
+
+	ret = regmap_bulk_write(data->regmap, CPUID_MCA_CMD,
+				&input, CPUID_WR_REG_LEN);
+	if (ret < 0)
+		goto exit_unlock;
+
+	ret = sbrmi_wait_status(data, &hw_status, HW_ALERT_MASK);
+	if (ret < 0)
+		goto exit_unlock;
+
+	ret = regmap_bulk_read(data->regmap, CPUID_MCA_CMD,
+			       &output, CPUID_RD_REG_LEN);
+	if (ret < 0)
+		goto exit_unlock;
+
+	ret = regmap_write(data->regmap, SBRMI_STATUS,
+			   HW_ALERT_MASK);
+	if (ret < 0)
+		goto exit_unlock;
+
+	if (output.num_bytes != CPUID_RD_REG_LEN - 1) {
+		ret = -EMSGSIZE;
+		goto exit_unlock;
+	}
+	if (output.status) {
+		ret = -EPROTOTYPE;
+		msg->fw_ret_code = output.status;
+		goto exit_unlock;
+	}
+	msg->data_out.cpu_msr_out = output.value;
+exit_unlock:
+	return ret;
+}
 
 static int sbrmi_clear_status_alert(struct sbrmi_data *data)
 {
@@ -58,15 +284,15 @@ static int sbrmi_clear_status_alert(struct sbrmi_data *data)
 			    SW_ALERT_MASK);
 }
 
-int rmi_mailbox_xfer(struct sbrmi_data *data,
-		     struct sbrmi_mailbox_msg *msg)
+static int __rmi_mailbox_xfer(struct sbrmi_data *data,
+			      struct apml_message *msg)
 {
-	unsigned int bytes;
-	int i, ret, retry = 10;
+	unsigned int bytes, ec;
+	int i, ret;
 	int sw_status;
 	u8 byte;
 
-	mutex_lock(&data->lock);
+	msg->fw_ret_code = 0;
 
 	ret = sbrmi_clear_status_alert(data);
 	if (ret < 0)
@@ -87,8 +313,8 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	 * Command Data In[31:0] to SBRMI::InBndMsg_inst[4:1]
 	 * SBRMI_x3C(MSB):SBRMI_x39(LSB)
 	 */
-	for (i = 0; i < 4; i++) {
-		byte = (msg->data_in >> i * 8) & 0xff;
+	for (i = 0; i < MB_DATA_SIZE; i++) {
+		byte = msg->data_in.reg_in[i];
 		ret = regmap_write(data->regmap, SBRMI_INBNDMSG1 + i, byte);
 		if (ret < 0)
 			goto exit_unlock;
@@ -107,46 +333,182 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	 * an ALERT (if enabled) to initiator (BMC) to indicate completion
 	 * of the requested command
 	 */
-	do {
-		ret = regmap_read(data->regmap, SBRMI_STATUS, &sw_status);
-		if (sw_status < 0) {
-			ret = sw_status;
-			goto exit_unlock;
-		}
-		if (sw_status & SW_ALERT_MASK)
-			break;
-		usleep_range(50, 100);
-	} while (retry--);
-
-	if (retry < 0) {
-		ret = -EIO;
+	ret = sbrmi_wait_status(data, &sw_status, SW_ALERT_MASK);
+	if (ret < 0)
 		goto exit_unlock;
-	}
+
+	ret = regmap_read(data->regmap, SBRMI_OUTBNDMSG7, &ec);
+	if (ret || ec)
+		goto exit_clear_alert;
 
 	/*
 	 * For a read operation, the initiator (BMC) reads the firmware
 	 * response Command Data Out[31:0] from SBRMI::OutBndMsg_inst[4:1]
 	 * {SBRMI_x34(MSB):SBRMI_x31(LSB)}.
 	 */
-	if (msg->read) {
-		for (i = 0; i < 4; i++) {
+	if (msg->data_in.reg_in[RD_FLAG_INDEX]) {
+		for (i = 0; i < MB_DATA_SIZE; i++) {
 			ret = regmap_read(data->regmap,
 					  SBRMI_OUTBNDMSG1 + i, &bytes);
 			if (ret < 0)
-				goto exit_unlock;
-			msg->data_out |= bytes << i * 8;
+				break;
+			msg->data_out.reg_out[i] = bytes;
 		}
 	}
-
+exit_clear_alert:
 	/*
 	 * BMC must write 1'b1 to SBRMI::Status[SwAlertSts] to clear the
 	 * ALERT to initiator
 	 */
 	ret = regmap_write(data->regmap, SBRMI_STATUS,
-			   sw_status | SW_ALERT_MASK);
-
+			   SW_ALERT_MASK);
+	if (ec) {
+		ret = -EPROTOTYPE;
+		msg->fw_ret_code = ec;
+	}
 exit_unlock:
+	return ret;
+}
+
+static int rmi_xfer(struct sbrmi_data *data,
+		    struct apml_message *msg)
+{
+	int ret;
+
+	if (!data || !msg)
+		return -ENODEV;
+
+	/*
+	 * Only one I2C transaction can happen at
+	 * one time. Take lock across so no two protocol is
+	 * invoked at same time, modifying the register value.
+	 */
+	mutex_lock(&data->lock);
+	/* Verify device unbind/remove is not invoked */
+	if (atomic_read(&data->no_new_trans)) {
+		mutex_unlock(&data->lock);
+		return -EBUSY;
+	}
+
+	/*
+	 * Set the in_progress variable to true, to wait for
+	 * completion during unbind/remove of driver
+	 */
+	atomic_set(&data->in_progress, 1);
+
+	switch (msg->cmd) {
+	case 0 ... 0x999:
+		/* Mailbox protocol */
+		ret = __rmi_mailbox_xfer(data, msg);
+		break;
+	case APML_CPUID:
+		ret = rmi_cpuid_read(data, msg);
+		break;
+	case APML_MCA_MSR:
+		/* MCAMSR protocol */
+		ret = rmi_mca_msr_read(data, msg);
+		break;
+	case APML_REG:
+		/* REG R/W */
+		if (msg->data_in.reg_in[RD_FLAG_INDEX])
+			ret = regmap_read(data->regmap,
+					  msg->data_in.reg_in[REG_OFF_INDEX],
+					  &msg->data_out.mb_out[RD_WR_DATA_INDEX]);
+		else
+			ret = regmap_write(data->regmap,
+					   msg->data_in.reg_in[REG_OFF_INDEX],
+					   msg->data_in.reg_in[REG_VAL_INDEX]);
+		break;
+	default:
+		break;
+	}
+	/* Send complete only if device is unbinded/remove */
+	if (atomic_read(&data->no_new_trans))
+		complete(&data->misc_fops_done);
+
+	atomic_set(&data->in_progress, 0);
 	mutex_unlock(&data->lock);
+
 	return ret;
 }
+
+/*
+ * rmi_mailbox_xfer() function is used in hwmon/sbrmi.c
+ * To support backward compatibility for the hwmon driver
+ * will export the rmi_mailbox_xfer function and call the
+ * new function rmi_xfer from it.
+ */
+int rmi_mailbox_xfer(struct sbrmi_data *data,
+		     struct apml_message *msg)
+{
+	return rmi_xfer(data, msg);
+}
 EXPORT_SYMBOL(rmi_mailbox_xfer);
+
+static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+	int __user *arguser = (int  __user *)arg;
+	struct apml_message msg = { 0 };
+	bool read = false;
+	int ret = -EFAULT;
+
+	struct sbrmi_data *data = container_of(fp->private_data, struct sbrmi_data,
+					       sbrmi_misc_dev);
+	if (!data)
+		return -ENODEV;
+
+	/*
+	 * If device remove/unbind is called do not allow new transaction
+	 */
+	if (atomic_read(&data->no_new_trans))
+		return -EBUSY;
+	/* Copy the structure from user */
+	if (copy_struct_from_user(&msg, sizeof(msg), arguser,
+				  sizeof(struct apml_message)))
+		return ret;
+
+	/* Is this a read/monitor/get request */
+	if (msg.data_in.reg_in[RD_FLAG_INDEX])
+		read = true;
+
+	ret = rmi_xfer(data, &msg);
+
+	/* Copy results back to user only for get/monitor commands and firmware failures */
+	if ((read && !ret) || ret == -EPROTOTYPE) {
+		if (copy_to_user(arguser, &msg, sizeof(struct apml_message)))
+			ret = -EFAULT;
+	}
+	return ret;
+}
+
+static const struct file_operations sbrmi_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= sbrmi_ioctl,
+	.compat_ioctl	= sbrmi_ioctl,
+};
+
+int create_misc_rmi_device(struct sbrmi_data *data,
+			   struct device *dev)
+{
+	int ret;
+
+	data->sbrmi_misc_dev.name	= devm_kasprintf(dev,
+							 GFP_KERNEL,
+							 "sbrmi-%x",
+							 data->dev_static_addr);
+	data->sbrmi_misc_dev.minor	= MISC_DYNAMIC_MINOR;
+	data->sbrmi_misc_dev.fops	= &sbrmi_fops;
+	data->sbrmi_misc_dev.parent	= dev;
+	data->sbrmi_misc_dev.nodename	= devm_kasprintf(dev,
+							 GFP_KERNEL,
+							 "sbrmi-%x",
+							 data->dev_static_addr);
+	data->sbrmi_misc_dev.mode	= 0600;
+
+	ret = misc_register(&data->sbrmi_misc_dev);
+	if (ret)
+		return ret;
+
+	dev_info(dev, "register %s device\n", data->sbrmi_misc_dev.name);
+	return ret;
+}
diff --git a/drivers/misc/amd-sb/sbrmi-core.h b/drivers/misc/amd-sb/sbrmi-core.h
new file mode 100644
index 000000000000..acab439cd728
--- /dev/null
+++ b/drivers/misc/amd-sb/sbrmi-core.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2021-2024 Advanced Micro Devices, Inc.
+ */
+
+#ifndef _SBRMI_CORE__H_
+#define _SBRMI_CORE__H_
+
+/* SB-RMI registers */
+enum sbrmi_reg {
+	SBRMI_REV		= 0x00,
+	SBRMI_CTRL,
+	SBRMI_STATUS,
+	SBRMI_OUTBNDMSG0	= 0x30,
+	SBRMI_OUTBNDMSG1,
+	SBRMI_OUTBNDMSG2,
+	SBRMI_OUTBNDMSG3,
+	SBRMI_OUTBNDMSG4,
+	SBRMI_OUTBNDMSG5,
+	SBRMI_OUTBNDMSG6,
+	SBRMI_OUTBNDMSG7,
+	SBRMI_INBNDMSG0,
+	SBRMI_INBNDMSG1,
+	SBRMI_INBNDMSG2,
+	SBRMI_INBNDMSG3,
+	SBRMI_INBNDMSG4,
+	SBRMI_INBNDMSG5,
+	SBRMI_INBNDMSG6,
+	SBRMI_INBNDMSG7,
+	SBRMI_SW_INTERRUPT,
+	SBRMI_THREAD128CS	= 0x4b,
+};
+
+int create_misc_rmi_device(struct sbrmi_data *data,
+			   struct device *dev);
+#endif /*_SBRMI_CORE__H_*/
diff --git a/drivers/misc/amd-sb/sbrmi-i2c.c b/drivers/misc/amd-sb/sbrmi-i2c.c
index 539539c42754..f0499b0ef5e3 100644
--- a/drivers/misc/amd-sb/sbrmi-i2c.c
+++ b/drivers/misc/amd-sb/sbrmi-i2c.c
@@ -15,8 +15,9 @@
 #include <linux/of.h>
 #include <linux/regmap.h>
 #include <misc/amd-sb.h>
+#include "sbrmi-core.h"
 
-#define SBRMI_CTRL	0x1
+#define MAX_WAIT_TIME_SEC	(3)
 
 static int sbrmi_enable_alert(struct sbrmi_data *data)
 {
@@ -40,15 +41,15 @@ static int sbrmi_enable_alert(struct sbrmi_data *data)
 
 static int sbrmi_get_max_pwr_limit(struct sbrmi_data *data)
 {
-	struct sbrmi_mailbox_msg msg = { 0 };
+	struct apml_message msg = { 0 };
 	int ret;
 
 	msg.cmd = SBRMI_READ_PKG_MAX_PWR_LIMIT;
-	msg.read = true;
+	msg.data_in.reg_in[RD_FLAG_INDEX] = 1;
 	ret = rmi_mailbox_xfer(data, &msg);
 	if (ret < 0)
 		return ret;
-	data->pwr_limit_max = msg.data_out;
+	data->pwr_limit_max = msg.data_out.mb_out[RD_WR_DATA_INDEX];
 
 	return ret;
 }
@@ -67,6 +68,8 @@ static int sbrmi_i2c_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
+	atomic_set(&data->in_progress, 0);
+	atomic_set(&data->no_new_trans, 0);
 	mutex_init(&data->lock);
 
 	data->regmap = devm_regmap_init_i2c(client, &sbrmi_i2c_regmap_config);
@@ -83,7 +86,10 @@ static int sbrmi_i2c_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
+	data->dev_static_addr = client->addr;
+	init_completion(&data->misc_fops_done);
 	dev_set_drvdata(dev, (void *)data);
+
 	data->pdev = platform_device_register_data(dev, "sbrmi-hwmon",
 						   PLATFORM_DEVID_NONE,
 						   data,
@@ -92,6 +98,11 @@ static int sbrmi_i2c_probe(struct i2c_client *client)
 		pr_err("unable to register platform device for sbrmi-hwmon\n");
 		return PTR_ERR(data->pdev);
 	}
+
+	ret = create_misc_rmi_device(data, dev);
+	if (ret)
+		platform_device_unregister(data->pdev);
+
 	return ret;
 }
 
@@ -102,6 +113,26 @@ static void sbrmi_i2c_remove(struct i2c_client *client)
 	if (!data)
 		return;
 	platform_device_unregister(data->pdev);
+	/*
+	 * Set the no_new_trans so no new transaction can
+	 * occur in sbrmi_ioctl
+	 */
+	atomic_set(&data->no_new_trans, 1);
+	/*
+	 * If any transaction is in progress wait for the
+	 * transaction to get complete
+	 * Max wait is 3 sec for any pending transaction to
+	 * complete.
+	 */
+	if (atomic_read(&data->in_progress))
+		wait_for_completion_timeout(&data->misc_fops_done,
+					    MAX_WAIT_TIME_SEC * HZ);
+	misc_deregister(&data->sbrmi_misc_dev);
+	/* Assign fops and parent of misc dev to NULL */
+	data->sbrmi_misc_dev.fops = NULL;
+	data->sbrmi_misc_dev.parent = NULL;
+	dev_info(&client->dev, "Removed sbrmi driver\n");
+	return;
 }
 
 static const struct i2c_device_id sbrmi_id[] = {
diff --git a/include/misc/amd-sb.h b/include/misc/amd-sb.h
index 71abb3035f48..30585cb0f480 100644
--- a/include/misc/amd-sb.h
+++ b/include/misc/amd-sb.h
@@ -6,9 +6,11 @@
 #ifndef _AMD_SB_H_
 #define _AMD_SB_H_
 
+#include <linux/miscdevice.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <uapi/linux/amd-apml.h>
 /*
  * SB-RMI supports soft mailbox service request to MP1 (power management
  * firmware) through SBRMI inbound/outbound message registers.
@@ -21,25 +23,33 @@ enum sbrmi_msg_id {
 	SBRMI_READ_PKG_MAX_PWR_LIMIT,
 };
 
-/* Each client has this additional data */
+/*
+ * Each client has this additional data
+ * in_progress: set during any transaction, mailbox/cpuid/mcamsr/readreg,
+ * to indicate a transaction is in progress.
+ * no_new_trans: set in rmmod/unbind path to indicate,
+ * not to accept new transactions
+ */
 struct sbrmi_data {
+	struct completion misc_fops_done;
+	struct miscdevice sbrmi_misc_dev;
 	struct regmap *regmap;
+	/* Mutex locking */
 	struct mutex lock;
 	struct platform_device *pdev;
+	atomic_t in_progress;
+	atomic_t no_new_trans;
 	u32 pwr_limit_max;
+	u8 dev_static_addr;
+	u8 rev;
 } __packed;
 
-struct sbrmi_mailbox_msg {
-	u8 cmd;
-	bool read;
-	u32 data_in;
-	u32 data_out;
-};
-
 #if IS_ENABLED(CONFIG_AMD_SBRMI_I2C)
-int rmi_mailbox_xfer(struct sbrmi_data *data, struct sbrmi_mailbox_msg *msg);
+int rmi_mailbox_xfer(struct sbrmi_data *data,
+		     struct apml_message *msg);
 #else
-int rmi_mailbox_xfer(struct sbrmi_data *data, struct sbrmi_mailbox_msg *msg)
+int rmi_mailbox_xfer(struct sbrmi_data *data,
+		     struct apml_message *msg)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/include/uapi/linux/amd-apml.h b/include/uapi/linux/amd-apml.h
new file mode 100644
index 000000000000..11d34ee83b87
--- /dev/null
+++ b/include/uapi/linux/amd-apml.h
@@ -0,0 +1,74 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (C) 2021-2024 Advanced Micro Devices, Inc.
+ */
+#ifndef _AMD_APML_H_
+#define _AMD_APML_H_
+
+#include <linux/types.h>
+
+enum apml_protocol {
+	APML_CPUID	= 0x1000,
+	APML_MCA_MSR,
+	APML_REG,
+};
+
+/* These are byte indexes into data_in and data_out arrays */
+#define RD_WR_DATA_INDEX	0
+#define REG_OFF_INDEX		0
+#define REG_VAL_INDEX		4
+#define THREAD_LOW_INDEX	4
+#define THREAD_HI_INDEX		5
+#define EXT_FUNC_INDEX		6
+#define RD_FLAG_INDEX		7
+
+#define MB_DATA_SIZE		4
+
+struct apml_message {
+	/* message ids:
+	 * Mailbox Messages:	0x0 ... 0x999
+	 * APML_CPUID:		0x1000
+	 * APML_MCA_MSR:	0x1001
+	 * APML_REG:		0x1002
+	 */
+	__u32 cmd;
+
+	/*
+	 * 8 bit data for reg read,
+	 * 32 bit data in case of mailbox,
+	 * up to 64 bit in case of cpuid and mca msr
+	 */
+	union {
+		__u64 cpu_msr_out;
+		__u32 mb_out[2];
+		__u8 reg_out[8];
+	} data_out;
+
+	/*
+	 * [0]...[3] mailbox 32bit input
+	 *	     cpuid & mca msr,
+	 *	     rmi rd/wr: reg_offset
+	 * [4][5] cpuid & mca msr: thread
+	 * [4] rmi reg wr: value
+	 * [6] cpuid: ext function & read eax/ebx or ecx/edx
+	 *	[7:0] -> bits [7:4] -> ext function &
+	 *	bit [0] read eax/ebx or ecx/edx
+	 * [7] read/write functionality
+	 */
+	union {
+		__u64 cpu_msr_in;
+		__u32 mb_in[2];
+		__u8 reg_in[8];
+	} data_in;
+	/*
+	 * Status code is returned in case of CPUID/MCA access
+	 * Error code is returned in case of soft mailbox
+	 */
+	__u32 fw_ret_code;
+} __attribute__((packed));
+
+/* ioctl command for mailbox msgs using generic _IOWR */
+#define SBRMI_BASE_IOCTL_NR      0xF9
+#define SBRMI_IOCTL_CMD          _IOWR(SBRMI_BASE_IOCTL_NR, 0, struct apml_message)
+
+#endif /*_AMD_APML_H_*/
-- 
2.25.1

[PATCH 6/6] misc: sbrmi: Add support for new revision

[Akshay Gupta]

- RMI device supports v21 on Turin, which has increased register set.
  Hence, requires 2byte for register addressing.

- Both Genoa and Turin processors are SP5 compatible, often CPUs are
  interchanged on the base boards with BMC remaining the same.
  Hence, we need to identify correct regmap configuration. A mechanism
  was not defined to identify the RMI register address width.
- The address width can only be determined if the socket is powered ON.
- This patch also addresses CPUID and MCAMSR register read protocol,
  the modification is due to the increase in register address size and
  the modified thread input.

Signed-off-by: Akshay Gupta <akshay.gupta@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
 drivers/misc/amd-sb/sbrmi-core.c | 215 +++++++++++++++++++++++--------
 drivers/misc/amd-sb/sbrmi-i2c.c  |  94 ++++++++++++--
 2 files changed, 242 insertions(+), 67 deletions(-)

diff --git a/drivers/misc/amd-sb/sbrmi-core.c b/drivers/misc/amd-sb/sbrmi-core.c
index c180f58b92c3..dc6f4e806a91 100644
--- a/drivers/misc/amd-sb/sbrmi-core.c
+++ b/drivers/misc/amd-sb/sbrmi-core.c
@@ -29,13 +29,17 @@
 /* MSR */
 #define MSR_RD_REG_LEN		0xa
 #define MSR_WR_REG_LEN		0x8
+#define MSR_WR_REG_LEN_v21	0x9
 #define MSR_RD_DATA_LEN		0x8
 #define MSR_WR_DATA_LEN		0x7
+#define MSR_WR_DATA_LEN_v21	0x8
 /* CPUID */
 #define CPUID_RD_DATA_LEN	0x8
 #define CPUID_WR_DATA_LEN	0x8
+#define CPUID_WR_DATA_LEN_v21	0x9
 #define CPUID_RD_REG_LEN	0xa
 #define CPUID_WR_REG_LEN	0x9
+#define CPUID_WR_REG_LEN_v21	0xa
 
 /* CPUID MSR Command Ids */
 #define CPUID_MCA_CMD	0x73
@@ -55,6 +59,19 @@ struct cpu_msr_indata {
 	u8 ext; /* extended function */
 } __packed;
 
+/* input for bulk write to v21 of CPUID and MSR protocol */
+struct cpu_msr_indata_v21 {
+	u8 wr_len;	/* const value */
+	u8 rd_len;	/* const value */
+	u8 proto_cmd;	/* const value */
+	u16 thread;	/* thread number */
+	union {
+		u8 reg_offset[4];	/* input value */
+		u32 value;
+	};
+	u8 ext; /* extended function */
+} __packed;
+
 /* output for bulk read from CPUID and MSR protocol */
 struct cpu_msr_outdata {
 	u8 num_bytes;	/* number of bytes return */
@@ -65,16 +82,16 @@ struct cpu_msr_outdata {
 	};
 } __packed;
 
-#define prepare_mca_msr_input_message(input, thread_id, data_in)	\
+#define prepare_mca_msr_input_message(input, thread_id, data_in, wr_data_len)	\
 	input.rd_len = MSR_RD_DATA_LEN,					\
-	input.wr_len = MSR_WR_DATA_LEN,					\
+	input.wr_len = wr_data_len,					\
 	input.proto_cmd = RD_MCA_CMD,					\
 	input.thread = thread_id << 1,					\
 	input.value =  data_in
 
-#define prepare_cpuid_input_message(input, thread_id, func, ext_func)	\
+#define prepare_cpuid_input_message(input, thread_id, func, ext_func, wr_data_len)	\
 	input.rd_len = CPUID_RD_DATA_LEN,				\
-	input.wr_len = CPUID_WR_DATA_LEN,				\
+	input.wr_len = wr_data_len,				\
 	input.proto_cmd = RD_CPUID_CMD,					\
 	input.thread = thread_id << 1,					\
 	input.value =  func,						\
@@ -131,25 +148,13 @@ static int sbrmi_wait_status(struct sbrmi_data *data,
 }
 
 /* MCA MSR protocol */
-static int rmi_mca_msr_read(struct sbrmi_data *data,
-			    struct apml_message *msg)
+static int msr_datain_v20(struct sbrmi_data *data,
+			  struct apml_message *msg)
 {
-	struct cpu_msr_outdata output = {0};
 	struct cpu_msr_indata input = {0};
 	int ret, val = 0;
-	int hw_status;
 	u16 thread;
 
-	/* cache the rev value to identify if protocol is supported or not */
-	if (!data->rev) {
-		ret = sbrmi_get_rev(data);
-		if (ret < 0)
-			return ret;
-	}
-	/* MCA MSR protocol for REV 0x10 is not supported*/
-	if (data->rev == 0x10)
-		return -EOPNOTSUPP;
-
 	thread = msg->data_in.reg_in[THREAD_LOW_INDEX] |
 		 msg->data_in.reg_in[THREAD_HI_INDEX] << 8;
 
@@ -160,15 +165,71 @@ static int rmi_mca_msr_read(struct sbrmi_data *data,
 	}
 	ret = regmap_write(data->regmap, SBRMI_THREAD128CS, val);
 	if (ret < 0)
-		goto exit_unlock;
+		return ret;
 
 	prepare_mca_msr_input_message(input, thread,
-				      msg->data_in.mb_in[RD_WR_DATA_INDEX]);
+				      msg->data_in.mb_in[RD_WR_DATA_INDEX],
+				      MSR_WR_DATA_LEN);
 
 	ret = regmap_bulk_write(data->regmap, CPUID_MCA_CMD,
 				&input, MSR_WR_REG_LEN);
-	if (ret < 0)
-		goto exit_unlock;
+	return ret;
+}
+
+static int msr_datain_v21(struct sbrmi_data *data,
+			  struct apml_message *msg)
+{
+	struct cpu_msr_indata_v21 input = {0};
+	int ret;
+	u16 thread;
+
+	thread = msg->data_in.reg_in[THREAD_LOW_INDEX] |
+		 msg->data_in.reg_in[THREAD_HI_INDEX] << 8;
+
+	prepare_mca_msr_input_message(input, thread,
+				      msg->data_in.mb_in[RD_WR_DATA_INDEX],
+				      MSR_WR_DATA_LEN_v21);
+
+	ret = regmap_bulk_write(data->regmap, CPUID_MCA_CMD,
+				&input, MSR_WR_REG_LEN_v21);
+	return ret;
+}
+
+static int rmi_mca_msr_read(struct sbrmi_data *data,
+			    struct apml_message *msg)
+{
+	struct cpu_msr_outdata output = {0};
+	int ret;
+	int hw_status;
+
+	if (!data->regmap)
+		return -ENODEV;
+
+	/* cache the rev value to identify if protocol is supported or not */
+	if (!data->rev) {
+		ret = sbrmi_get_rev(data);
+		if (ret < 0)
+			return ret;
+	}
+
+	switch (data->rev) {
+	/* MCA MSR protocol for REV 0x10 is not supported*/
+	case 0x10:
+		return -EOPNOTSUPP;
+	case 0x20:
+		ret = msr_datain_v20(data, msg);
+		if (ret < 0)
+			goto exit_unlock;
+
+		break;
+	case 0x21:
+		ret = msr_datain_v21(data, msg);
+		if (ret < 0)
+			goto exit_unlock;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
 
 	ret = sbrmi_wait_status(data, &hw_status, HW_ALERT_MASK);
 	if (ret < 0)
@@ -200,25 +261,13 @@ static int rmi_mca_msr_read(struct sbrmi_data *data,
 }
 
 /* Read CPUID function protocol */
-static int rmi_cpuid_read(struct sbrmi_data *data,
-			  struct apml_message *msg)
+static int cpuid_datain_v20(struct sbrmi_data *data,
+			    struct apml_message *msg)
 {
 	struct cpu_msr_indata input = {0};
-	struct cpu_msr_outdata output = {0};
-	int val = 0;
-	int ret, hw_status;
+	int ret, val = 0;
 	u16 thread;
 
-	/* cache the rev value to identify if protocol is supported or not */
-	if (!data->rev) {
-		ret = sbrmi_get_rev(data);
-		if (ret < 0)
-			return ret;
-	}
-	/* CPUID protocol for REV 0x10 is not supported*/
-	if (data->rev == 0x10)
-		return -EOPNOTSUPP;
-
 	thread = msg->data_in.reg_in[THREAD_LOW_INDEX] |
 		 msg->data_in.reg_in[THREAD_HI_INDEX] << 8;
 
@@ -229,16 +278,71 @@ static int rmi_cpuid_read(struct sbrmi_data *data,
 	}
 	ret = regmap_write(data->regmap, SBRMI_THREAD128CS, val);
 	if (ret < 0)
-		goto exit_unlock;
-
+		return ret;
 	prepare_cpuid_input_message(input, thread,
 				    msg->data_in.mb_in[RD_WR_DATA_INDEX],
-				    msg->data_in.reg_in[EXT_FUNC_INDEX]);
+				    msg->data_in.reg_in[EXT_FUNC_INDEX],
+				    CPUID_WR_DATA_LEN);
 
 	ret = regmap_bulk_write(data->regmap, CPUID_MCA_CMD,
 				&input, CPUID_WR_REG_LEN);
-	if (ret < 0)
-		goto exit_unlock;
+	return ret;
+}
+
+static int cpuid_datain_v21(struct sbrmi_data *data,
+			    struct apml_message *msg)
+{
+	struct cpu_msr_indata_v21 input = {0};
+	int ret;
+	u16 thread;
+
+	thread = msg->data_in.reg_in[THREAD_LOW_INDEX] |
+		 msg->data_in.reg_in[THREAD_HI_INDEX] << 8;
+
+	prepare_cpuid_input_message(input, thread,
+				    msg->data_in.mb_in[RD_WR_DATA_INDEX],
+				    msg->data_in.reg_in[EXT_FUNC_INDEX],
+				    CPUID_WR_DATA_LEN_v21);
+
+	ret = regmap_bulk_write(data->regmap, CPUID_MCA_CMD,
+				&input, CPUID_WR_REG_LEN_v21);
+	return ret;
+}
+
+/* CPUID protocol */
+static int rmi_cpuid_read(struct sbrmi_data *data,
+			  struct apml_message *msg)
+{
+	struct cpu_msr_outdata output = {0};
+	int ret, hw_status;
+
+	if (!data->regmap)
+		return -ENODEV;
+
+	/* cache the rev value to identify if protocol is supported or not */
+	if (!data->rev) {
+		ret = sbrmi_get_rev(data);
+		if (ret < 0)
+			return ret;
+	}
+
+	switch (data->rev) {
+	/* CPUID protocol for REV 0x10 is not supported*/
+	case 0x10:
+		return -EOPNOTSUPP;
+	case 0x20:
+		ret = cpuid_datain_v20(data, msg);
+		if (ret < 0)
+			goto exit_unlock;
+		break;
+	case 0x21:
+		ret = cpuid_datain_v21(data, msg);
+		if (ret < 0)
+			goto exit_unlock;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
 
 	ret = sbrmi_wait_status(data, &hw_status, HW_ALERT_MASK);
 	if (ret < 0)
@@ -412,11 +516,11 @@ static int rmi_xfer(struct sbrmi_data *data,
 		/* REG R/W */
 		if (msg->data_in.reg_in[RD_FLAG_INDEX])
 			ret = regmap_read(data->regmap,
-					  msg->data_in.reg_in[REG_OFF_INDEX],
+					  msg->data_in.mb_in[REG_OFF_INDEX],
 					  &msg->data_out.mb_out[RD_WR_DATA_INDEX]);
 		else
 			ret = regmap_write(data->regmap,
-					   msg->data_in.reg_in[REG_OFF_INDEX],
+					   msg->data_in.mb_in[REG_OFF_INDEX],
 					   msg->data_in.reg_in[REG_VAL_INDEX]);
 		break;
 	default:
@@ -466,7 +570,6 @@ static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 	if (copy_struct_from_user(&msg, sizeof(msg), arguser,
 				  sizeof(struct apml_message)))
 		return ret;
-
 	/* Is this a read/monitor/get request */
 	if (msg.data_in.reg_in[RD_FLAG_INDEX])
 		read = true;
@@ -492,18 +595,18 @@ int create_misc_rmi_device(struct sbrmi_data *data,
 {
 	int ret;
 
-	data->sbrmi_misc_dev.name	= devm_kasprintf(dev,
-							 GFP_KERNEL,
-							 "sbrmi-%x",
-							 data->dev_static_addr);
-	data->sbrmi_misc_dev.minor	= MISC_DYNAMIC_MINOR;
-	data->sbrmi_misc_dev.fops	= &sbrmi_fops;
-	data->sbrmi_misc_dev.parent	= dev;
+	data->sbrmi_misc_dev.name		= devm_kasprintf(dev,
+								 GFP_KERNEL,
+								 "sbrmi-%x",
+								 data->dev_static_addr);
+	data->sbrmi_misc_dev.minor		= MISC_DYNAMIC_MINOR;
+	data->sbrmi_misc_dev.fops		= &sbrmi_fops;
+	data->sbrmi_misc_dev.parent		= dev;
 	data->sbrmi_misc_dev.nodename	= devm_kasprintf(dev,
-							 GFP_KERNEL,
-							 "sbrmi-%x",
-							 data->dev_static_addr);
-	data->sbrmi_misc_dev.mode	= 0600;
+								 GFP_KERNEL,
+								 "sbrmi-%x",
+								 data->dev_static_addr);
+	data->sbrmi_misc_dev.mode		= 0600;
 
 	ret = misc_register(&data->sbrmi_misc_dev);
 	if (ret)
diff --git a/drivers/misc/amd-sb/sbrmi-i2c.c b/drivers/misc/amd-sb/sbrmi-i2c.c
index f0499b0ef5e3..f0645b3fa05b 100644
--- a/drivers/misc/amd-sb/sbrmi-i2c.c
+++ b/drivers/misc/amd-sb/sbrmi-i2c.c
@@ -19,6 +19,82 @@
 
 #define MAX_WAIT_TIME_SEC	(3)
 
+/* Try 2 byte address size before switching to 1 byte */
+#define MAX_RETRY	5
+
+/*
+ * There are processor with RMI address size as 2 byte and others with 1 byte,
+ * with no clear detection mechanism in place.
+ * Execute 2 bytes access first, and then fall back to 1 byte.
+ * Sending 2 bytes first is safer as sending 1 byte address size on
+ * Turin(2 bytes) can cause unrecoverable error and may requries to reboot
+ * the system.
+ */
+static int configure_regmap(struct i2c_client *client, struct sbrmi_data *data)
+{
+	struct regmap_config sbrmi_i2c_regmap_config = {
+		.reg_bits = 8,
+		.val_bits = 8,
+	};
+	struct regmap_config sbrmi_i2c_regmap_config_2_bytes = {
+		.reg_bits = 16,
+		.val_bits = 8,
+		.reg_format_endian = REGMAP_ENDIAN_LITTLE,
+	};
+	u32 i;
+	int ret, rev;
+
+	if (!client || !data)
+		return -ENODEV;
+
+	/* Checking RMI register address size as 2 bytes */
+	data->regmap = devm_regmap_init_i2c(client,
+					    &sbrmi_i2c_regmap_config_2_bytes);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	for (i = 0; i < MAX_RETRY; i++) {
+		ret = regmap_read(data->regmap, SBRMI_REV, &rev);
+		if (ret < 0) {
+			usleep_range(10000, 20000);
+			continue;
+		} else {
+			break;
+		}
+	}
+
+	/*
+	 * Fall back to register address size as 1 byte
+	 * Check for revision as 0x21 to not break backward compatbility
+	 * for few Family:0x19 processors
+	 */
+	if (ret || rev != 0x21) {
+		data->regmap = devm_regmap_init_i2c(client,
+						    &sbrmi_i2c_regmap_config);
+		if (IS_ERR(data->regmap))
+			return PTR_ERR(data->regmap);
+
+		ret = regmap_read(data->regmap, SBRMI_REV, &rev);
+		if (ret < 0)
+			return ret;
+	}
+	data->rev = rev;
+
+	/*
+	 * For some Turin platforms, first 1 byte transaction can be successful,
+	 * verify if revision is 0x21, if yes, switch to 2 byte address size
+	 */
+	if (data->rev == 0x21)
+		data->regmap = devm_regmap_init_i2c(client,
+						    &sbrmi_i2c_regmap_config_2_bytes);
+	else
+		data->regmap = devm_regmap_init_i2c(client,
+						    &sbrmi_i2c_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+	return ret;
+}
+
 static int sbrmi_enable_alert(struct sbrmi_data *data)
 {
 	int ctrl, ret;
@@ -58,24 +134,20 @@ static int sbrmi_i2c_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct sbrmi_data *data;
-	struct regmap_config sbrmi_i2c_regmap_config = {
-		.reg_bits = 8,
-		.val_bits = 8,
-	};
 	int ret;
 
 	data = devm_kzalloc(dev, sizeof(struct sbrmi_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
+	ret = configure_regmap(client, data);
+	if (ret)
+		return -EPROBE_DEFER;
+
 	atomic_set(&data->in_progress, 0);
 	atomic_set(&data->no_new_trans, 0);
 	mutex_init(&data->lock);
 
-	data->regmap = devm_regmap_init_i2c(client, &sbrmi_i2c_regmap_config);
-	if (IS_ERR(data->regmap))
-		return PTR_ERR(data->regmap);
-
 	/* Enable alert for SB-RMI sequence */
 	ret = sbrmi_enable_alert(data);
 	if (ret < 0)
@@ -91,9 +163,9 @@ static int sbrmi_i2c_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, (void *)data);
 
 	data->pdev = platform_device_register_data(dev, "sbrmi-hwmon",
-						   PLATFORM_DEVID_NONE,
-						   data,
-						   sizeof(struct sbrmi_data));
+						      PLATFORM_DEVID_NONE,
+						      data,
+						      sizeof(struct sbrmi_data));
 	if (IS_ERR(data->pdev)) {
 		pr_err("unable to register platform device for sbrmi-hwmon\n");
 		return PTR_ERR(data->pdev);
-- 
2.25.1

Re: [PATCH 1/6] hwmon/misc sbrmi: Move core sbrmi from hwmon to misc

[Greg KH]

On Thu, Jul 04, 2024 at 11:16:19AM +0000, Akshay Gupta wrote:
> --- a/drivers/hwmon/sbrmi.c
> +++ b/drivers/hwmon/sbrmi.c
> @@ -3,190 +3,18 @@
>   * sbrmi.c - hwmon driver for a SB-RMI mailbox
>   *           compliant AMD SoC device.
>   *
> - * Copyright (C) 2020-2021 Advanced Micro Devices, Inc.
> + * Copyright (C) 2020-2024 Advanced Micro Devices, Inc.

Are you sure this shouldn't be "2020-2021,2024"?

thanks,

greg k-h

Re: [PATCH 0/6] misc: add amd side-band functionality

[Greg KH]

On Thu, Jul 04, 2024 at 11:16:18AM +0000, Akshay Gupta wrote:
> AMD's APML interface provides system management functionality accessed by
> the BMC. Sbrmi driver under hwmon subsystem, which is probed as an i2c
> driver and reports power using APML specified protocol.

What is "APML"?  What is "BMC"?

> However, APML interface defines few other protocols to support
> full system management functionality out-of-band.

What is out-of-band here?

> This patchset is an attempt to add all APML core functionality in one place
> and provide hwmon and user interface
> 1. Move the i2c client probe and sbrmi core functionality from drivers/hwmon
>    to drivers/misc/
> 2. Add a platform device, which probes the hwmon/sbrmi and continues to
>    report power using the symbol exported by the misc/sbrmi-core.

Are you sure this is a platform device?  It better not be a pci one...

> 3. Convert i2c to regmap which provides multiple benefits
>    over direct smbus APIs.

What are those?

> 4. Register a misc device which provides 
>     a. An ioctl interface through node /dev/sbrmiX

Why?

>     b. Open-sourced and widely used https://github.com/amd/esmi_oob_library
>        will continue to provide user-space programmable API.

Will this use the new ioctl api?  If it's not present, what will it use
instead?

thanks,

greg k-h

Re: [PATCH 2/6] misc: sbrmi: Add platform device add to create platform device

[Greg KH]

On Thu, Jul 04, 2024 at 11:16:20AM +0000, Akshay Gupta wrote:
> - AMD provides socket power information from out of band
>   which can be read by sensors.
> - platform driver will probe drivers/hwmon/sbrmi as a platform device
>   and share the sbrmi device data.

So you are "splitting" a real device into different ones using a
platform device?  THat's not ok, and an abuse of the platform api.
Please use the correct one for that instead.


> 
> Signed-off-by: Akshay Gupta <akshay.gupta@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
>  drivers/misc/amd-sb/sbrmi-i2c.c | 25 ++++++++++++++++++++++++-
>  include/misc/amd-sb.h           |  2 ++
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/amd-sb/sbrmi-i2c.c b/drivers/misc/amd-sb/sbrmi-i2c.c
> index c4903d9e9f0f..b593bbdd78e0 100644
> --- a/drivers/misc/amd-sb/sbrmi-i2c.c
> +++ b/drivers/misc/amd-sb/sbrmi-i2c.c
> @@ -72,7 +72,29 @@ static int sbrmi_i2c_probe(struct i2c_client *client)
>  		return ret;
>  
>  	/* Cache maximum power limit */
> -	return sbrmi_get_max_pwr_limit(data);
> +	ret = sbrmi_get_max_pwr_limit(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_set_drvdata(dev, (void *)data);

No need to cast, right?

> +	data->pdev = platform_device_register_data(dev, "sbrmi-hwmon",
> +						   PLATFORM_DEVID_NONE,

Yeah, that's not ok.  Please do this correctly, as this is NOT a
platform device, but rather a made-up one that you just created out of
no where.  Instead use the correct apis for that.

> +						   data,
> +						   sizeof(struct sbrmi_data));
> +	if (IS_ERR(data->pdev)) {
> +		pr_err("unable to register platform device for sbrmi-hwmon\n");
> +		return PTR_ERR(data->pdev);

You don't need to unwind anything else here?



> +	}
> +	return ret;
> +}
> +
> +static void sbrmi_i2c_remove(struct i2c_client *client)
> +{
> +	struct sbrmi_data *data = dev_get_drvdata(&client->dev);
> +
> +	if (!data)
> +		return;

How can that happen?

thanks,

greg k-h

Re: [PATCH 3/6] misc: sbrmi: Use regmap subsystem

[Greg KH]

On Thu, Jul 04, 2024 at 11:16:21AM +0000, Akshay Gupta wrote:
> - regmap subsystem provides multiple benefits over direct smbus APIs

In what way, please document.

> - The susbsytem can be helpful in following cases
>   - Differnet types of bus (i2c/i3c)
>   - Different Register address size (1byte/2byte)

Is that what is happening here?  I don't see i3c support...

> --- a/include/misc/amd-sb.h
> +++ b/include/misc/amd-sb.h
> @@ -7,8 +7,8 @@
>  #define _AMD_SB_H_
>  
>  #include <linux/mutex.h>
> -#include <linux/i2c.h>

Why remove this?

>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>  /*
>   * SB-RMI supports soft mailbox service request to MP1 (power management
>   * firmware) through SBRMI inbound/outbound message registers.
> @@ -23,11 +23,11 @@ enum sbrmi_msg_id {
>  
>  /* Each client has this additional data */
>  struct sbrmi_data {
> -	struct i2c_client *client;
> +	struct regmap *regmap;
>  	struct mutex lock;
>  	struct platform_device *pdev;
>  	u32 pwr_limit_max;
> -};
> +} __packed;

Why is this suddenly required to be __packed?

Isn't that going to cause more problems than it is worth?  And why is it
worth it at all?

thanks,

greg k-h

Re: [PATCH 6/6] misc: sbrmi: Add support for new revision

[Greg KH]

On Thu, Jul 04, 2024 at 11:16:24AM +0000, Akshay Gupta wrote:
> - RMI device supports v21 on Turin, which has increased register set.
>   Hence, requires 2byte for register addressing.
> 
> - Both Genoa and Turin processors are SP5 compatible, often CPUs are
>   interchanged on the base boards with BMC remaining the same.
>   Hence, we need to identify correct regmap configuration. A mechanism
>   was not defined to identify the RMI register address width.
> - The address width can only be determined if the socket is powered ON.
> - This patch also addresses CPUID and MCAMSR register read protocol,
>   the modification is due to the increase in register address size and
>   the modified thread input.
> 

When you have to list all of the different things you did in a single
change, that's a huge hint that it needs to be split up.

Remember, each patch can only do _one_ thing.

Would you want to review something that attempted to do all of this at
once?

thanks,

greg k-h

Re: [PATCH 1/6] hwmon/misc sbrmi: Move core sbrmi from hwmon to misc

[Gupta, Akshay]

On 7/4/2024 5:17 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, Jul 04, 2024 at 11:16:19AM +0000, Akshay Gupta wrote:
>> --- a/drivers/hwmon/sbrmi.c
>> +++ b/drivers/hwmon/sbrmi.c
>> @@ -3,190 +3,18 @@
>>    * sbrmi.c - hwmon driver for a SB-RMI mailbox
>>    *           compliant AMD SoC device.
>>    *
>> - * Copyright (C) 2020-2021 Advanced Micro Devices, Inc.
>> + * Copyright (C) 2020-2024 Advanced Micro Devices, Inc.
> Are you sure this shouldn't be "2020-2021,2024"?
>
> thanks,
>
> greg k-h

Thanks, I will check and update.

Regards,

Akshay

Re: [PATCH 2/6] misc: sbrmi: Add platform device add to create platform device

[Gupta, Akshay]

On 7/4/2024 5:20 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, Jul 04, 2024 at 11:16:20AM +0000, Akshay Gupta wrote:
>> - AMD provides socket power information from out of band
>>    which can be read by sensors.
>> - platform driver will probe drivers/hwmon/sbrmi as a platform device
>>    and share the sbrmi device data.
> So you are "splitting" a real device into different ones using a
> platform device?  THat's not ok, and an abuse of the platform api.
> Please use the correct one for that instead.

is it ok to call API, devm_hwmon_device_register_with_info() from misc 
driver

which will require to defining the hwmon ops in here.

>
>> Signed-off-by: Akshay Gupta <akshay.gupta@amd.com>
>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>> ---
>>   drivers/misc/amd-sb/sbrmi-i2c.c | 25 ++++++++++++++++++++++++-
>>   include/misc/amd-sb.h           |  2 ++
>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/amd-sb/sbrmi-i2c.c b/drivers/misc/amd-sb/sbrmi-i2c.c
>> index c4903d9e9f0f..b593bbdd78e0 100644
>> --- a/drivers/misc/amd-sb/sbrmi-i2c.c
>> +++ b/drivers/misc/amd-sb/sbrmi-i2c.c
>> @@ -72,7 +72,29 @@ static int sbrmi_i2c_probe(struct i2c_client *client)
>>                return ret;
>>
>>        /* Cache maximum power limit */
>> -     return sbrmi_get_max_pwr_limit(data);
>> +     ret = sbrmi_get_max_pwr_limit(data);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     dev_set_drvdata(dev, (void *)data);
> No need to cast, right?
Yes, will update.
>
>> +     data->pdev = platform_device_register_data(dev, "sbrmi-hwmon",
>> +                                                PLATFORM_DEVID_NONE,
> Yeah, that's not ok.  Please do this correctly, as this is NOT a
> platform device, but rather a made-up one that you just created out of
> no where.  Instead use the correct apis for that.
>
>> +                                                data,
>> +                                                sizeof(struct sbrmi_data));
>> +     if (IS_ERR(data->pdev)) {
>> +             pr_err("unable to register platform device for sbrmi-hwmon\n");
>> +             return PTR_ERR(data->pdev);
> You don't need to unwind anything else here?
Yes, not required.
>
>
>
>> +     }
>> +     return ret;
>> +}
>> +
>> +static void sbrmi_i2c_remove(struct i2c_client *client)
>> +{
>> +     struct sbrmi_data *data = dev_get_drvdata(&client->dev);
>> +
>> +     if (!data)
>> +             return;
> How can that happen?
Its just a safe check, once we remove platform driver this will not be 
required.
> greg k-h

Re: [PATCH 3/6] misc: sbrmi: Use regmap subsystem

[Gupta, Akshay]

On 7/4/2024 5:23 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, Jul 04, 2024 at 11:16:21AM +0000, Akshay Gupta wrote:
>> - regmap subsystem provides multiple benefits over direct smbus APIs
> In what way, please document.
Answered below.
>
>> - The susbsytem can be helpful in following cases
>>    - Differnet types of bus (i2c/i3c)
>>    - Different Register address size (1byte/2byte)
> Is that what is happening here?  I don't see i3c support...

This is a preparation for

1. I3C support: planning to add support, on AMD platforms BMC can 
communicate with SBRMI over I2C/I3C

2. patch 6/6 in patch series, is adding support for 2 bytes SBRMI 
register address size

>> --- a/include/misc/amd-sb.h
>> +++ b/include/misc/amd-sb.h
>> @@ -7,8 +7,8 @@
>>   #define _AMD_SB_H_
>>
>>   #include <linux/mutex.h>
>> -#include <linux/i2c.h>
> Why remove this?
In this patch "struct i2c_client" is removed, so no need for the header 
inclusion.
>
>>   #include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>>   /*
>>    * SB-RMI supports soft mailbox service request to MP1 (power management
>>    * firmware) through SBRMI inbound/outbound message registers.
>> @@ -23,11 +23,11 @@ enum sbrmi_msg_id {
>>
>>   /* Each client has this additional data */
>>   struct sbrmi_data {
>> -     struct i2c_client *client;
>> +     struct regmap *regmap;
>>        struct mutex lock;
>>        struct platform_device *pdev;
>>        u32 pwr_limit_max;
>> -};
>> +} __packed;
> Why is this suddenly required to be __packed?
>
> Isn't that going to cause more problems than it is worth?  And why is it
> worth it at all?
I will look into this and update.
>
> thanks,
>
> greg k-h

Re: [PATCH 6/6] misc: sbrmi: Add support for new revision

[Gupta, Akshay]

On 7/4/2024 5:24 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, Jul 04, 2024 at 11:16:24AM +0000, Akshay Gupta wrote:
>> - RMI device supports v21 on Turin, which has increased register set.
>>    Hence, requires 2byte for register addressing.
>>
>> - Both Genoa and Turin processors are SP5 compatible, often CPUs are
>>    interchanged on the base boards with BMC remaining the same.
>>    Hence, we need to identify correct regmap configuration. A mechanism
>>    was not defined to identify the RMI register address width.
>> - The address width can only be determined if the socket is powered ON.
>> - This patch also addresses CPUID and MCAMSR register read protocol,
>>    the modification is due to the increase in register address size and
>>    the modified thread input.
>>
> When you have to list all of the different things you did in a single
> change, that's a huge hint that it needs to be split up.
>
> Remember, each patch can only do _one_ thing.
>
> Would you want to review something that attempted to do all of this at
> once?
>
> thanks,
>
> greg k-h
I will split and resubmit,
thanks, Akshay

Re: [PATCH 0/6] misc: add amd side-band functionality

[Gupta, Akshay]

On 7/4/2024 5:18 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, Jul 04, 2024 at 11:16:18AM +0000, Akshay Gupta wrote:
>> AMD's APML interface provides system management functionality accessed by
>> the BMC. Sbrmi driver under hwmon subsystem, which is probed as an i2c
>> driver and reports power using APML specified protocol.
> What is "APML"?  What is "BMC"?
My bad, I will provide expansion for the acronyms in the first usage.
>
>> However, APML interface defines few other protocols to support
>> full system management functionality out-of-band.
> What is out-of-band here?

Out-of-band management is term used for Baseboard Management Controller 
(BMC) talking to

system management unit(IP in the processor, AMD's documentation called 
this link as side band interface.

We are using the terms out-of-band and side band interface as per context.

>
>> This patchset is an attempt to add all APML core functionality in one place
>> and provide hwmon and user interface
>> 1. Move the i2c client probe and sbrmi core functionality from drivers/hwmon
>>     to drivers/misc/
>> 2. Add a platform device, which probes the hwmon/sbrmi and continues to
>>     report power using the symbol exported by the misc/sbrmi-core.
> Are you sure this is a platform device?  It better not be a pci one...
It is not a pci device, will remove the platform device and register 
hwmon device in misc.
>
>> 3. Convert i2c to regmap which provides multiple benefits
>>     over direct smbus APIs.
> What are those?
We are planning to add i2c/i3c support and 1byte/2 byte register addressing.
>
>> 4. Register a misc device which provides
>>      a. An ioctl interface through node /dev/sbrmiX
> Why?
>
>>      b. Open-sourced and widely used https://github.com/amd/esmi_oob_library
>>         will continue to provide user-space programmable API.
> Will this use the new ioctl api?  If it's not present, what will it use
> instead?

This is an attempt to upstream the out of tree kernel module(which 
defines this ioctl), open sourced at:

https://github.com/amd/apml_modules/ and continue to support 
https://github.com/amd/esmi_oob_library

which depends only on this ioctl.

Thanks,

Akshay

> thanks,
>
> greg k-h