* [PATCH 1/5] hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD
2024-05-30 11:23 [PATCH 0/5] mfd: add amd side-band functionality Naveen Krishna Chatradhi
@ 2024-05-30 11:23 ` Naveen Krishna Chatradhi
2024-06-10 13:24 ` Guenter Roeck
2024-05-30 11:23 ` [PATCH 2/5] mfd: sbrmi: Add mfd cell in the probe Naveen Krishna Chatradhi
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Naveen Krishna Chatradhi @ 2024-05-30 11:23 UTC (permalink / raw)
To: linux-hwmon, inux-kernel
Cc: linux, lee, gregkh, arnd, Akshay Gupta, Akshay Gupta,
Naveen Krishna Chatradhi
From: Akshay Gupta <akshay.gupta@amd.com>
This is done to support other functionality provided by the SBRMI, which
does not fit in the hwmon subsystem.
- Move the core SBRMI core functionality and I2C device probing to MFD
- Modify this module to be probed as a platform device by the MFD cell.
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 | 245 +++----------------------------------
drivers/mfd/Kconfig | 10 +-
drivers/mfd/Makefile | 2 +
drivers/mfd/sbrmi-core.c | 137 +++++++++++++++++++++
drivers/mfd/sbrmi-i2c.c | 106 ++++++++++++++++
include/linux/mfd/amd-sb.h | 45 +++++++
7 files changed, 316 insertions(+), 230 deletions(-)
create mode 100644 drivers/mfd/sbrmi-core.c
create mode 100644 drivers/mfd/sbrmi-i2c.c
create mode 100644 include/linux/mfd/amd-sb.h
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e14ae18a973b..56b73e665424 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 MFD_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..aaeb5050eb0c 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/mfd/amd-sb.h>
#include <linux/module.h>
-#include <linux/mutex.h>
-#include <linux/of.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.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)
@@ -282,76 +110,35 @@ 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;
-
- 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;
+ struct sbrmi_data *data = dev_get_drvdata(pdev->dev.parent);
- /* 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,
+ 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/mfd/Kconfig b/drivers/mfd/Kconfig
index 266b4f54af60..0411cb29b6df 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1442,6 +1442,15 @@ config MFD_STMPE
Touchscreen: stmpe-ts
ADC: stmpe-adc
+config MFD_SBRMI_I2C
+ tristate "APML SBRMI support"
+ depends on I2C
+ select MFD_CORE
+ 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 sbrmi.
+
menu "STMicroelectronics STMPE Interface Drivers"
depends on MFD_STMPE
@@ -2333,6 +2342,5 @@ config MFD_RSMU_SPI
This driver provides common support for accessing the device.
Additional drivers must be enabled in order to use the functionality
of the device.
-
endmenu
endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c66f07edcd0e..6bc807c2e5ed 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -284,3 +284,5 @@ rsmu-i2c-objs := rsmu_core.o rsmu_i2c.o
rsmu-spi-objs := rsmu_core.o rsmu_spi.o
obj-$(CONFIG_MFD_RSMU_I2C) += rsmu-i2c.o
obj-$(CONFIG_MFD_RSMU_SPI) += rsmu-spi.o
+apml_sbrmi-objs := sbrmi-i2c.o sbrmi-core.o
+obj-$(CONFIG_MFD_SBRMI_I2C) += apml_sbrmi.o
diff --git a/drivers/mfd/sbrmi-core.c b/drivers/mfd/sbrmi-core.c
new file mode 100644
index 000000000000..d872b5107b36
--- /dev/null
+++ b/drivers/mfd/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/mfd/amd-sb.h>
+#include <linux/mutex.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/mfd/sbrmi-i2c.c b/drivers/mfd/sbrmi-i2c.c
new file mode 100644
index 000000000000..96215e986740
--- /dev/null
+++ b/drivers/mfd/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/mfd/amd-sb.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.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-mfd"},
+ {}
+};
+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-mfd",
+ .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/linux/mfd/amd-sb.h b/include/linux/mfd/amd-sb.h
new file mode 100644
index 000000000000..7805f31fb6ea
--- /dev/null
+++ b/include/linux/mfd/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_MFD_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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 1/5] hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD
2024-05-30 11:23 ` [PATCH 1/5] hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD Naveen Krishna Chatradhi
@ 2024-06-10 13:24 ` Guenter Roeck
0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2024-06-10 13:24 UTC (permalink / raw)
To: Naveen Krishna Chatradhi, linux-hwmon, inux-kernel
Cc: lee, gregkh, arnd, Akshay Gupta
On 5/30/24 04:23, Naveen Krishna Chatradhi wrote:
> From: Akshay Gupta <akshay.gupta@amd.com>
>
> This is done to support other functionality provided by the SBRMI, which
> does not fit in the hwmon subsystem.
>
> - Move the core SBRMI core functionality and I2C device probing to MFD
> - Modify this module to be probed as a platform device by the MFD cell.
>
> Signed-off-by: Akshay Gupta <Akshay.Gupta@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
For the hwmon part:
Acked-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/hwmon/Kconfig | 1 +
> drivers/hwmon/sbrmi.c | 245 +++----------------------------------
> drivers/mfd/Kconfig | 10 +-
> drivers/mfd/Makefile | 2 +
> drivers/mfd/sbrmi-core.c | 137 +++++++++++++++++++++
> drivers/mfd/sbrmi-i2c.c | 106 ++++++++++++++++
> include/linux/mfd/amd-sb.h | 45 +++++++
> 7 files changed, 316 insertions(+), 230 deletions(-)
> create mode 100644 drivers/mfd/sbrmi-core.c
> create mode 100644 drivers/mfd/sbrmi-i2c.c
> create mode 100644 include/linux/mfd/amd-sb.h
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index e14ae18a973b..56b73e665424 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 MFD_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..aaeb5050eb0c 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/mfd/amd-sb.h>
> #include <linux/module.h>
> -#include <linux/mutex.h>
> -#include <linux/of.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.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)
> @@ -282,76 +110,35 @@ 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;
> -
> - 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;
> + struct sbrmi_data *data = dev_get_drvdata(pdev->dev.parent);
>
> - /* 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,
> + 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/mfd/Kconfig b/drivers/mfd/Kconfig
> index 266b4f54af60..0411cb29b6df 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1442,6 +1442,15 @@ config MFD_STMPE
> Touchscreen: stmpe-ts
> ADC: stmpe-adc
>
> +config MFD_SBRMI_I2C
> + tristate "APML SBRMI support"
> + depends on I2C
> + select MFD_CORE
> + 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 sbrmi.
> +
> menu "STMicroelectronics STMPE Interface Drivers"
> depends on MFD_STMPE
>
> @@ -2333,6 +2342,5 @@ config MFD_RSMU_SPI
> This driver provides common support for accessing the device.
> Additional drivers must be enabled in order to use the functionality
> of the device.
> -
> endmenu
> endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index c66f07edcd0e..6bc807c2e5ed 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -284,3 +284,5 @@ rsmu-i2c-objs := rsmu_core.o rsmu_i2c.o
> rsmu-spi-objs := rsmu_core.o rsmu_spi.o
> obj-$(CONFIG_MFD_RSMU_I2C) += rsmu-i2c.o
> obj-$(CONFIG_MFD_RSMU_SPI) += rsmu-spi.o
> +apml_sbrmi-objs := sbrmi-i2c.o sbrmi-core.o
> +obj-$(CONFIG_MFD_SBRMI_I2C) += apml_sbrmi.o
> diff --git a/drivers/mfd/sbrmi-core.c b/drivers/mfd/sbrmi-core.c
> new file mode 100644
> index 000000000000..d872b5107b36
> --- /dev/null
> +++ b/drivers/mfd/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/mfd/amd-sb.h>
> +#include <linux/mutex.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/mfd/sbrmi-i2c.c b/drivers/mfd/sbrmi-i2c.c
> new file mode 100644
> index 000000000000..96215e986740
> --- /dev/null
> +++ b/drivers/mfd/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/mfd/amd-sb.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.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-mfd"},
> + {}
> +};
> +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-mfd",
> + .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/linux/mfd/amd-sb.h b/include/linux/mfd/amd-sb.h
> new file mode 100644
> index 000000000000..7805f31fb6ea
> --- /dev/null
> +++ b/include/linux/mfd/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_MFD_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_*/
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/5] mfd: sbrmi: Add mfd cell in the probe
2024-05-30 11:23 [PATCH 0/5] mfd: add amd side-band functionality Naveen Krishna Chatradhi
2024-05-30 11:23 ` [PATCH 1/5] hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD Naveen Krishna Chatradhi
@ 2024-05-30 11:23 ` Naveen Krishna Chatradhi
2024-05-30 11:23 ` [PATCH 3/5] mfd/hwmon sbrmi: Use regmap subsystem Naveen Krishna Chatradhi
` (3 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Naveen Krishna Chatradhi @ 2024-05-30 11:23 UTC (permalink / raw)
To: linux-hwmon, inux-kernel
Cc: linux, lee, gregkh, arnd, Akshay Gupta, Akshay Gupta,
Naveen Krishna Chatradhi
From: Akshay Gupta <akshay.gupta@amd.com>
- AMD provides socket power information from out of band
which can be read by sensors.
- mfd cell will probe the 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/mfd/sbrmi-i2c.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/sbrmi-i2c.c b/drivers/mfd/sbrmi-i2c.c
index 96215e986740..c19f0b3eb0cd 100644
--- a/drivers/mfd/sbrmi-i2c.c
+++ b/drivers/mfd/sbrmi-i2c.c
@@ -10,6 +10,7 @@
#include <linux/err.h>
#include <linux/i2c.h>
#include <linux/init.h>
+#include <linux/mfd/core.h>
#include <linux/mfd/amd-sb.h>
#include <linux/module.h>
#include <linux/mutex.h>
@@ -17,6 +18,10 @@
#define SBRMI_CTRL 0x1
+static struct mfd_cell apml_sbrmi[] = {
+ { .name = "sbrmi-hwmon" },
+};
+
static int sbrmi_enable_alert(struct i2c_client *client)
{
int ctrl;
@@ -72,7 +77,17 @@ 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);
+
+ ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, apml_sbrmi,
+ ARRAY_SIZE(apml_sbrmi), NULL, 0, NULL);
+ if (ret)
+ dev_err(dev, "Failed to register for sub-devices: %d\n", ret);
+ return ret;
}
static const struct i2c_device_id sbrmi_id[] = {
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 3/5] mfd/hwmon sbrmi: Use regmap subsystem
2024-05-30 11:23 [PATCH 0/5] mfd: add amd side-band functionality Naveen Krishna Chatradhi
2024-05-30 11:23 ` [PATCH 1/5] hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD Naveen Krishna Chatradhi
2024-05-30 11:23 ` [PATCH 2/5] mfd: sbrmi: Add mfd cell in the probe Naveen Krishna Chatradhi
@ 2024-05-30 11:23 ` Naveen Krishna Chatradhi
2024-06-10 13:31 ` Guenter Roeck
2024-05-30 11:23 ` [PATCH 4/5] mfd: sbrmi: Clear sbrmi status register bit SwAlertSts Naveen Krishna Chatradhi
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Naveen Krishna Chatradhi @ 2024-05-30 11:23 UTC (permalink / raw)
To: linux-hwmon, inux-kernel
Cc: linux, lee, gregkh, arnd, Akshay Gupta, Akshay Gupta,
Naveen Krishna Chatradhi
From: Akshay Gupta <akshay.gupta@amd.com>
- 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/mfd/Kconfig | 2 +-
drivers/mfd/sbrmi-core.c | 30 ++++++++++++------------------
drivers/mfd/sbrmi-i2c.c | 25 ++++++++++++++++---------
include/linux/mfd/amd-sb.h | 6 +++---
4 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 0411cb29b6df..d89513e5a06b 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1444,8 +1444,8 @@ config MFD_STMPE
config MFD_SBRMI_I2C
tristate "APML SBRMI support"
- depends on I2C
select MFD_CORE
+ select REGMAP_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
diff --git a/drivers/mfd/sbrmi-core.c b/drivers/mfd/sbrmi-core.c
index d872b5107b36..5617b91787ba 100644
--- a/drivers/mfd/sbrmi-core.c
+++ b/drivers/mfd/sbrmi-core.c
@@ -7,9 +7,9 @@
*/
#include <linux/delay.h>
#include <linux/err.h>
-#include <linux/i2c.h>
#include <linux/mfd/amd-sb.h>
#include <linux/mutex.h>
+#include <linux/regmap.h>
/* Mask for Status Register bit[1] */
#define SW_ALERT_MASK 0x2
@@ -44,6 +44,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 +52,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 +68,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 +77,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 +87,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 +98,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 +109,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 +121,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/mfd/sbrmi-i2c.c b/drivers/mfd/sbrmi-i2c.c
index c19f0b3eb0cd..bdf15a7a2167 100644
--- a/drivers/mfd/sbrmi-i2c.c
+++ b/drivers/mfd/sbrmi-i2c.c
@@ -15,6 +15,7 @@
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/of.h>
+#include <linux/regmap.h>
#define SBRMI_CTRL 0x1
@@ -22,22 +23,21 @@ static struct mfd_cell apml_sbrmi[] = {
{ .name = "sbrmi-hwmon" },
};
-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;
@@ -62,17 +62,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/linux/mfd/amd-sb.h b/include/linux/mfd/amd-sb.h
index 7805f31fb6ea..977b8228ffa1 100644
--- a/include/linux/mfd/amd-sb.h
+++ b/include/linux/mfd/amd-sb.h
@@ -7,7 +7,7 @@
#define _AMD_SB_H_
#include <linux/mutex.h>
-#include <linux/i2c.h>
+#include <linux/regmap.h>
/*
* SB-RMI supports soft mailbox service request to MP1 (power management
* firmware) through SBRMI inbound/outbound message registers.
@@ -22,10 +22,10 @@ enum sbrmi_msg_id {
/* Each client has this additional data */
struct sbrmi_data {
- struct i2c_client *client;
+ struct regmap *regmap;
struct mutex lock;
u32 pwr_limit_max;
-};
+} __packed;
struct sbrmi_mailbox_msg {
u8 cmd;
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/5] mfd/hwmon sbrmi: Use regmap subsystem
2024-05-30 11:23 ` [PATCH 3/5] mfd/hwmon sbrmi: Use regmap subsystem Naveen Krishna Chatradhi
@ 2024-06-10 13:31 ` Guenter Roeck
0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2024-06-10 13:31 UTC (permalink / raw)
To: Naveen Krishna Chatradhi, linux-hwmon, inux-kernel
Cc: lee, gregkh, arnd, Akshay Gupta
On 5/30/24 04:23, Naveen Krishna Chatradhi wrote:
> From: Akshay Gupta <akshay.gupta@amd.com>
>
> - 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>
The "hwmon" in the subject should be dropped.
> ---
> drivers/mfd/Kconfig | 2 +-
> drivers/mfd/sbrmi-core.c | 30 ++++++++++++------------------
> drivers/mfd/sbrmi-i2c.c | 25 ++++++++++++++++---------
> include/linux/mfd/amd-sb.h | 6 +++---
> 4 files changed, 32 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 0411cb29b6df..d89513e5a06b 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1444,8 +1444,8 @@ config MFD_STMPE
>
> config MFD_SBRMI_I2C
> tristate "APML SBRMI support"
> - depends on I2C
> select MFD_CORE
> + select REGMAP_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
> diff --git a/drivers/mfd/sbrmi-core.c b/drivers/mfd/sbrmi-core.c
> index d872b5107b36..5617b91787ba 100644
> --- a/drivers/mfd/sbrmi-core.c
> +++ b/drivers/mfd/sbrmi-core.c
> @@ -7,9 +7,9 @@
> */
> #include <linux/delay.h>
> #include <linux/err.h>
> -#include <linux/i2c.h>
> #include <linux/mfd/amd-sb.h>
> #include <linux/mutex.h>
> +#include <linux/regmap.h>
>
> /* Mask for Status Register bit[1] */
> #define SW_ALERT_MASK 0x2
> @@ -44,6 +44,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 +52,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 +68,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 +77,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 +87,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;
This is wrong. It should be
if (ret < 0)
goto exit_unlock;
> goto exit_unlock;
> @@ -102,8 +98,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 +109,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 +121,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/mfd/sbrmi-i2c.c b/drivers/mfd/sbrmi-i2c.c
> index c19f0b3eb0cd..bdf15a7a2167 100644
> --- a/drivers/mfd/sbrmi-i2c.c
> +++ b/drivers/mfd/sbrmi-i2c.c
> @@ -15,6 +15,7 @@
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/of.h>
> +#include <linux/regmap.h>
>
> #define SBRMI_CTRL 0x1
>
> @@ -22,22 +23,21 @@ static struct mfd_cell apml_sbrmi[] = {
> { .name = "sbrmi-hwmon" },
> };
>
> -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;
> @@ -62,17 +62,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/linux/mfd/amd-sb.h b/include/linux/mfd/amd-sb.h
> index 7805f31fb6ea..977b8228ffa1 100644
> --- a/include/linux/mfd/amd-sb.h
> +++ b/include/linux/mfd/amd-sb.h
> @@ -7,7 +7,7 @@
> #define _AMD_SB_H_
>
> #include <linux/mutex.h>
> -#include <linux/i2c.h>
> +#include <linux/regmap.h>
> /*
> * SB-RMI supports soft mailbox service request to MP1 (power management
> * firmware) through SBRMI inbound/outbound message registers.
> @@ -22,10 +22,10 @@ enum sbrmi_msg_id {
>
> /* Each client has this additional data */
> struct sbrmi_data {
> - struct i2c_client *client;
> + struct regmap *regmap;
> struct mutex lock;
> u32 pwr_limit_max;
> -};
> +} __packed;
>
> struct sbrmi_mailbox_msg {
> u8 cmd;
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/5] mfd: sbrmi: Clear sbrmi status register bit SwAlertSts
2024-05-30 11:23 [PATCH 0/5] mfd: add amd side-band functionality Naveen Krishna Chatradhi
` (2 preceding siblings ...)
2024-05-30 11:23 ` [PATCH 3/5] mfd/hwmon sbrmi: Use regmap subsystem Naveen Krishna Chatradhi
@ 2024-05-30 11:23 ` Naveen Krishna Chatradhi
2024-05-30 11:23 ` [PATCH 5/5] mfd/hwmon: sbrmi: Add support for APML protocols Naveen Krishna Chatradhi
2024-06-13 17:05 ` [PATCH 0/5] mfd: add amd side-band functionality Lee Jones
5 siblings, 0 replies; 15+ messages in thread
From: Naveen Krishna Chatradhi @ 2024-05-30 11:23 UTC (permalink / raw)
To: linux-hwmon, inux-kernel
Cc: linux, lee, gregkh, arnd, Akshay Gupta, Akshay Gupta,
Naveen Krishna Chatradhi
From: Akshay Gupta <akshay.gupta@amd.com>
- 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/mfd/sbrmi-core.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/mfd/sbrmi-core.c b/drivers/mfd/sbrmi-core.c
index 5617b91787ba..d5fbe5676cab 100644
--- a/drivers/mfd/sbrmi-core.c
+++ b/drivers/mfd/sbrmi-core.c
@@ -41,6 +41,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)
{
@@ -51,6 +67,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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 5/5] mfd/hwmon: sbrmi: Add support for APML protocols
2024-05-30 11:23 [PATCH 0/5] mfd: add amd side-band functionality Naveen Krishna Chatradhi
` (3 preceding siblings ...)
2024-05-30 11:23 ` [PATCH 4/5] mfd: sbrmi: Clear sbrmi status register bit SwAlertSts Naveen Krishna Chatradhi
@ 2024-05-30 11:23 ` Naveen Krishna Chatradhi
2024-06-10 13:39 ` Guenter Roeck
2024-06-13 17:05 ` [PATCH 0/5] mfd: add amd side-band functionality Lee Jones
5 siblings, 1 reply; 15+ messages in thread
From: Naveen Krishna Chatradhi @ 2024-05-30 11:23 UTC (permalink / raw)
To: linux-hwmon, inux-kernel
Cc: linux, lee, gregkh, arnd, Akshay Gupta, Akshay Gupta,
Naveen Krishna Chatradhi
From: Akshay Gupta <akshay.gupta@amd.com>
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 | 43 ++--
drivers/mfd/sbrmi-core.c | 461 +++++++++++++++++++++++++++++-----
drivers/mfd/sbrmi-core.h | 36 +++
drivers/mfd/sbrmi-i2c.c | 81 ++++--
include/linux/mfd/amd-sb.h | 32 ++-
include/uapi/linux/amd-apml.h | 74 ++++++
6 files changed, 615 insertions(+), 112 deletions(-)
create mode 100644 drivers/mfd/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 aaeb5050eb0c..a8bf9aea52f9 100644
--- a/drivers/hwmon/sbrmi.c
+++ b/drivers/hwmon/sbrmi.c
@@ -19,42 +19,46 @@
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_sbrmi_device *rmi_dev = dev_get_drvdata(dev);
+ struct apml_message msg = { 0 };
int ret;
if (type != hwmon_power)
return -EINVAL;
- msg.read = true;
+ mutex_lock(&rmi_dev->lock);
+ msg.data_in.reg_in[RD_FLAG_INDEX] = 1;
switch (attr) {
case hwmon_power_input:
msg.cmd = SBRMI_READ_PKG_PWR_CONSUMPTION;
- ret = rmi_mailbox_xfer(data, &msg);
+ ret = rmi_mailbox_xfer(rmi_dev, &msg);
break;
case hwmon_power_cap:
msg.cmd = SBRMI_READ_PKG_PWR_LIMIT;
- ret = rmi_mailbox_xfer(data, &msg);
+ ret = rmi_mailbox_xfer(rmi_dev, &msg);
break;
case hwmon_power_cap_max:
- msg.data_out = data->pwr_limit_max;
ret = 0;
+ msg.data_out.mb_out[RD_WR_DATA_INDEX] = rmi_dev->pwr_limit_max;
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;
+
+ mutex_unlock(&rmi_dev->lock);
return ret;
}
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_sbrmi_device *rmi_dev = dev_get_drvdata(dev);
+ struct apml_message msg = { 0 };
+ int ret;
if (type != hwmon_power && attr != hwmon_power_cap)
return -EINVAL;
@@ -64,13 +68,16 @@ static int sbrmi_write(struct device *dev, enum hwmon_sensor_types type,
*/
val /= 1000;
- val = clamp_val(val, SBRMI_PWR_MIN, data->pwr_limit_max);
+ val = clamp_val(val, SBRMI_PWR_MIN, rmi_dev->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);
+ mutex_lock(&rmi_dev->lock);
+ ret = rmi_mailbox_xfer(rmi_dev, &msg);
+ mutex_unlock(&rmi_dev->lock);
+ return ret;
}
static umode_t sbrmi_is_visible(const void *data,
@@ -114,9 +121,9 @@ static int sbrmi_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct device *hwmon_dev;
- struct sbrmi_data *data = dev_get_drvdata(pdev->dev.parent);
+ struct apml_sbrmi_device *rmi_dev = dev_get_drvdata(pdev->dev.parent);
- hwmon_dev = devm_hwmon_device_register_with_info(dev, "sbrmi", data,
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, "sbrmi", rmi_dev,
&sbrmi_chip_info, NULL);
return PTR_ERR_OR_ZERO(hwmon_dev);
}
diff --git a/drivers/mfd/sbrmi-core.c b/drivers/mfd/sbrmi-core.c
index d5fbe5676cab..05d6c68d78b6 100644
--- a/drivers/mfd/sbrmi-core.c
+++ b/drivers/mfd/sbrmi-core.c
@@ -7,45 +7,271 @@
*/
#include <linux/delay.h>
#include <linux/err.h>
+#include <linux/fs.h>
#include <linux/mfd/amd-sb.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/regmap.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 apml_sbrmi_device *rmi_dev)
+{
+ 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(rmi_dev->regmap,
+ msg.data_in.reg_in[REG_OFF_INDEX],
+ &msg.data_out.mb_out[RD_WR_DATA_INDEX]);
+ if (ret < 0)
+ return ret;
+
+ rmi_dev->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 apml_sbrmi_device *rmi_dev,
+ int *status, int mask)
+{
+ int ret, retry = 100;
+
+ do {
+ ret = regmap_read(rmi_dev->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 apml_sbrmi_device *rmi_dev,
+ 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 (!rmi_dev->rev) {
+ ret = sbrmi_get_rev(rmi_dev);
+ if (ret < 0)
+ return ret;
+ }
+ /* MCA MSR protocol for REV 0x10 is not supported*/
+ if (rmi_dev->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(rmi_dev->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(rmi_dev->regmap, CPUID_MCA_CMD,
+ &input, MSR_WR_REG_LEN);
+ if (ret < 0)
+ goto exit_unlock;
+
+ ret = sbrmi_wait_status(rmi_dev, &hw_status, HW_ALERT_MASK);
+ if (ret < 0)
+ goto exit_unlock;
+
+ ret = regmap_bulk_read(rmi_dev->regmap, CPUID_MCA_CMD,
+ &output, MSR_RD_REG_LEN);
+ if (ret < 0)
+ goto exit_unlock;
+
+ ret = regmap_write(rmi_dev->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 apml_sbrmi_device *rmi_dev,
+ 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 (!rmi_dev->rev) {
+ ret = sbrmi_get_rev(rmi_dev);
+ if (ret < 0)
+ return ret;
+ }
+ /* CPUID protocol for REV 0x10 is not supported*/
+ if (rmi_dev->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(rmi_dev->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(rmi_dev->regmap, CPUID_MCA_CMD,
+ &input, CPUID_WR_REG_LEN);
+ if (ret < 0)
+ goto exit_unlock;
+
+ ret = sbrmi_wait_status(rmi_dev, &hw_status, HW_ALERT_MASK);
+ if (ret < 0)
+ goto exit_unlock;
+
+ ret = regmap_bulk_read(rmi_dev->regmap, CPUID_MCA_CMD,
+ &output, CPUID_RD_REG_LEN);
+ if (ret < 0)
+ goto exit_unlock;
+
+ ret = regmap_write(rmi_dev->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)
+static int sbrmi_clear_status_alert(struct apml_sbrmi_device *rmi_dev)
{
int sw_status, ret;
- ret = regmap_read(data->regmap, SBRMI_STATUS,
+ ret = regmap_read(rmi_dev->regmap, SBRMI_STATUS,
&sw_status);
if (ret < 0)
return ret;
@@ -53,31 +279,31 @@ static int sbrmi_clear_status_alert(struct sbrmi_data *data)
if (!(sw_status & SW_ALERT_MASK))
return 0;
- return regmap_write(data->regmap, SBRMI_STATUS,
+ return regmap_write(rmi_dev->regmap, SBRMI_STATUS,
SW_ALERT_MASK);
}
-int rmi_mailbox_xfer(struct sbrmi_data *data,
- struct sbrmi_mailbox_msg *msg)
+int rmi_mailbox_xfer(struct apml_sbrmi_device *rmi_dev,
+ 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);
+ ret = sbrmi_clear_status_alert(rmi_dev);
if (ret < 0)
goto exit_unlock;
/* Indicate firmware a command is to be serviced */
- ret = regmap_write(data->regmap, SBRMI_INBNDMSG7, START_CMD);
+ ret = regmap_write(rmi_dev->regmap, SBRMI_INBNDMSG7, START_CMD);
if (ret < 0)
goto exit_unlock;
/* Write the command to SBRMI::InBndMsg_inst0 */
- ret = regmap_write(data->regmap, SBRMI_INBNDMSG0, msg->cmd);
+ ret = regmap_write(rmi_dev->regmap, SBRMI_INBNDMSG0, msg->cmd);
if (ret < 0)
goto exit_unlock;
@@ -86,9 +312,9 @@ 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;
- ret = regmap_write(data->regmap, SBRMI_INBNDMSG1 + i, byte);
+ for (i = 0; i < MB_DATA_SIZE; i++) {
+ byte = msg->data_in.reg_in[i];
+ ret = regmap_write(rmi_dev->regmap, SBRMI_INBNDMSG1 + i, byte);
if (ret < 0)
goto exit_unlock;
}
@@ -97,7 +323,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 = regmap_write(data->regmap, SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX);
+ ret = regmap_write(rmi_dev->regmap, SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX);
if (ret < 0)
goto exit_unlock;
@@ -106,46 +332,159 @@ 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(rmi_dev, &sw_status, SW_ALERT_MASK);
+ if (ret < 0)
goto exit_unlock;
- }
+
+ ret = regmap_read(rmi_dev->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++) {
- ret = regmap_read(data->regmap,
+ if (msg->data_in.reg_in[RD_FLAG_INDEX]) {
+ for (i = 0; i < MB_DATA_SIZE; i++) {
+ ret = regmap_read(rmi_dev->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);
-
+ ret = regmap_write(rmi_dev->regmap, SBRMI_STATUS,
+ SW_ALERT_MASK);
+ if (ec) {
+ ret = -EPROTOTYPE;
+ msg->fw_ret_code = ec;
+ }
exit_unlock:
- mutex_unlock(&data->lock);
return ret;
}
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 apml_sbrmi_device *rmi_dev = container_of(fp->private_data, struct apml_sbrmi_device,
+ sbrmi_misc_dev);
+ if (!rmi_dev)
+ return -ENODEV;
+
+ /*
+ * If device remove/unbind is called do not allow new transaction
+ */
+ if (atomic_read(&rmi_dev->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;
+
+ /*
+ * 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(&rmi_dev->lock);
+ /* Verify device unbind/remove is not invoked */
+ if (atomic_read(&rmi_dev->no_new_trans)) {
+ mutex_unlock(&rmi_dev->lock);
+ return -EBUSY;
+ }
+
+ /* Is this a read/monitor/get request */
+ if (msg.data_in.reg_in[RD_FLAG_INDEX])
+ read = true;
+
+ /*
+ * Set the in_progress variable to true, to wait for
+ * completion during unbind/remove of driver
+ */
+ atomic_set(&rmi_dev->in_progress, 1);
+
+ switch (msg.cmd) {
+ case 0 ... 0x999:
+ /* Mailbox protocol */
+ ret = rmi_mailbox_xfer(rmi_dev, &msg);
+ break;
+ case APML_CPUID:
+ ret = rmi_cpuid_read(rmi_dev, &msg);
+ break;
+ case APML_MCA_MSR:
+ /* MCAMSR protocol */
+ ret = rmi_mca_msr_read(rmi_dev, &msg);
+ break;
+ case APML_REG:
+ /* REG R/W */
+ if (read) {
+ ret = regmap_read(rmi_dev->regmap,
+ msg.data_in.reg_in[REG_OFF_INDEX],
+ &msg.data_out.mb_out[RD_WR_DATA_INDEX]);
+ } else {
+ ret = regmap_write(rmi_dev->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(&rmi_dev->no_new_trans))
+ complete(&rmi_dev->misc_fops_done);
+
+ atomic_set(&rmi_dev->in_progress, 0);
+ mutex_unlock(&rmi_dev->lock);
+
+ /* 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 apml_sbrmi_device *rmi_dev,
+ struct device *dev)
+{
+ int ret;
+
+ rmi_dev->sbrmi_misc_dev.name = devm_kasprintf(dev,
+ GFP_KERNEL,
+ "sbrmi-%x",
+ rmi_dev->dev_static_addr);
+ rmi_dev->sbrmi_misc_dev.minor = MISC_DYNAMIC_MINOR;
+ rmi_dev->sbrmi_misc_dev.fops = &sbrmi_fops;
+ rmi_dev->sbrmi_misc_dev.parent = dev;
+ rmi_dev->sbrmi_misc_dev.nodename = devm_kasprintf(dev,
+ GFP_KERNEL,
+ "sbrmi-%x",
+ rmi_dev->dev_static_addr);
+ rmi_dev->sbrmi_misc_dev.mode = 0600;
+
+ ret = misc_register(&rmi_dev->sbrmi_misc_dev);
+ if (ret)
+ return ret;
+
+ dev_info(dev, "register %s device\n", rmi_dev->sbrmi_misc_dev.name);
+ return ret;
+}
diff --git a/drivers/mfd/sbrmi-core.h b/drivers/mfd/sbrmi-core.h
new file mode 100644
index 000000000000..6339931d4eb3
--- /dev/null
+++ b/drivers/mfd/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 apml_sbrmi_device *rmi_dev,
+ struct device *dev);
+#endif /*_SBRMI_CORE__H_*/
diff --git a/drivers/mfd/sbrmi-i2c.c b/drivers/mfd/sbrmi-i2c.c
index bdf15a7a2167..a8dcb7a92af8 100644
--- a/drivers/mfd/sbrmi-i2c.c
+++ b/drivers/mfd/sbrmi-i2c.c
@@ -12,18 +12,18 @@
#include <linux/init.h>
#include <linux/mfd/core.h>
#include <linux/mfd/amd-sb.h>
-#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/regmap.h>
+#include "sbrmi-core.h"
-#define SBRMI_CTRL 0x1
+#define MAX_WAIT_TIME_SEC (3)
static struct mfd_cell apml_sbrmi[] = {
{ .name = "sbrmi-hwmon" },
};
-static int sbrmi_enable_alert(struct sbrmi_data *data)
+static int sbrmi_enable_alert(struct apml_sbrmi_device *rmi_dev)
{
int ctrl, ret;
@@ -31,29 +31,29 @@ static int sbrmi_enable_alert(struct sbrmi_data *data)
* Enable the SB-RMI Software alert status
* by writing 0 to bit 4 of Control register(0x1)
*/
- ret = regmap_read(data->regmap, SBRMI_CTRL, &ctrl);
+ ret = regmap_read(rmi_dev->regmap, SBRMI_CTRL, &ctrl);
if (ret < 0)
return ret;
if (ctrl & 0x10) {
ctrl &= ~0x10;
- return regmap_write(data->regmap, SBRMI_CTRL, ctrl);
+ return regmap_write(rmi_dev->regmap, SBRMI_CTRL, ctrl);
}
return 0;
}
-static int sbrmi_get_max_pwr_limit(struct sbrmi_data *data)
+static int sbrmi_get_max_pwr_limit(struct apml_sbrmi_device *rmi_dev)
{
- struct sbrmi_mailbox_msg msg = { 0 };
+ struct apml_message msg = { 0 };
int ret;
msg.cmd = SBRMI_READ_PKG_MAX_PWR_LIMIT;
- msg.read = true;
- ret = rmi_mailbox_xfer(data, &msg);
+ msg.data_in.reg_in[RD_FLAG_INDEX] = 1;
+ ret = rmi_mailbox_xfer(rmi_dev, &msg);
if (ret < 0)
return ret;
- data->pwr_limit_max = msg.data_out;
+ rmi_dev->pwr_limit_max = msg.data_out.mb_out[RD_WR_DATA_INDEX];
return ret;
}
@@ -61,40 +61,76 @@ static int sbrmi_get_max_pwr_limit(struct sbrmi_data *data)
static int sbrmi_i2c_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
- struct sbrmi_data *data;
+ struct apml_sbrmi_device *rmi_dev;
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)
+ rmi_dev = devm_kzalloc(dev, sizeof(struct apml_sbrmi_device), GFP_KERNEL);
+ if (!rmi_dev)
return -ENOMEM;
- mutex_init(&data->lock);
+ atomic_set(&rmi_dev->in_progress, 0);
+ atomic_set(&rmi_dev->no_new_trans, 0);
+ mutex_init(&rmi_dev->lock);
- data->regmap = devm_regmap_init_i2c(client, &sbrmi_i2c_regmap_config);
- if (IS_ERR(data->regmap))
- return PTR_ERR(data->regmap);
+ rmi_dev->regmap = devm_regmap_init_i2c(client, &sbrmi_i2c_regmap_config);
+ if (IS_ERR(rmi_dev->regmap))
+ return PTR_ERR(rmi_dev->regmap);
/* Enable alert for SB-RMI sequence */
- ret = sbrmi_enable_alert(data);
+ ret = sbrmi_enable_alert(rmi_dev);
if (ret < 0)
return ret;
/* Cache maximum power limit */
- ret = sbrmi_get_max_pwr_limit(data);
+ ret = sbrmi_get_max_pwr_limit(rmi_dev);
if (ret < 0)
return ret;
- dev_set_drvdata(dev, (void *)data);
+ dev_set_drvdata(dev, (void *)rmi_dev);
ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, apml_sbrmi,
ARRAY_SIZE(apml_sbrmi), NULL, 0, NULL);
- if (ret)
+ if (ret) {
dev_err(dev, "Failed to register for sub-devices: %d\n", ret);
- return ret;
+ return ret;
+ }
+
+ rmi_dev->dev_static_addr = client->addr;
+ init_completion(&rmi_dev->misc_fops_done);
+ return create_misc_rmi_device(rmi_dev, dev);
+}
+
+static void sbrmi_i2c_remove(struct i2c_client *client)
+{
+ struct apml_sbrmi_device *rmi_dev = dev_get_drvdata(&client->dev);
+
+ if (!rmi_dev)
+ return;
+ /*
+ * Set the no_new_trans so no new transaction can
+ * occur in sbrmi_ioctl
+ */
+ atomic_set(&rmi_dev->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(&rmi_dev->in_progress))
+ wait_for_completion_timeout(&rmi_dev->misc_fops_done,
+ MAX_WAIT_TIME_SEC * HZ);
+ misc_deregister(&rmi_dev->sbrmi_misc_dev);
+ /* Assign fops and parent of misc dev to NULL */
+ rmi_dev->sbrmi_misc_dev.fops = NULL;
+ rmi_dev->sbrmi_misc_dev.parent = NULL;
+ dev_info(&client->dev, "Removed sbrmi driver\n");
+
+ return;
}
static const struct i2c_device_id sbrmi_id[] = {
@@ -117,6 +153,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/linux/mfd/amd-sb.h b/include/linux/mfd/amd-sb.h
index 977b8228ffa1..7bb5b89bb71a 100644
--- a/include/linux/mfd/amd-sb.h
+++ b/include/linux/mfd/amd-sb.h
@@ -6,8 +6,10 @@
#ifndef _AMD_SB_H_
#define _AMD_SB_H_
+#include <linux/miscdevice.h>
#include <linux/mutex.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.
@@ -20,24 +22,32 @@ enum sbrmi_msg_id {
SBRMI_READ_PKG_MAX_PWR_LIMIT,
};
-/* Each client has this additional data */
-struct sbrmi_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 apml_sbrmi_device {
+ struct completion misc_fops_done;
+ struct miscdevice sbrmi_misc_dev;
struct regmap *regmap;
+ /* Mutex locking */
struct mutex lock;
+ 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_MFD_SBRMI_I2C)
-int rmi_mailbox_xfer(struct sbrmi_data *data, struct sbrmi_mailbox_msg *msg);
+int rmi_mailbox_xfer(struct apml_sbrmi_device *rmi_dev,
+ struct apml_message *msg);
#else
-int rmi_mailbox_xfer(struct sbrmi_data *data, struct sbrmi_mailbox_msg *msg)
+int rmi_mailbox_xfer(struct apml_sbrmi_device *rmi_dev,
+ 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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 5/5] mfd/hwmon: sbrmi: Add support for APML protocols
2024-05-30 11:23 ` [PATCH 5/5] mfd/hwmon: sbrmi: Add support for APML protocols Naveen Krishna Chatradhi
@ 2024-06-10 13:39 ` Guenter Roeck
0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2024-06-10 13:39 UTC (permalink / raw)
To: Naveen Krishna Chatradhi, linux-hwmon, inux-kernel
Cc: lee, gregkh, arnd, Akshay Gupta
On 5/30/24 04:23, Naveen Krishna Chatradhi wrote:
> From: Akshay Gupta <akshay.gupta@amd.com>
>
> 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 | 43 ++--
> drivers/mfd/sbrmi-core.c | 461 +++++++++++++++++++++++++++++-----
> drivers/mfd/sbrmi-core.h | 36 +++
> drivers/mfd/sbrmi-i2c.c | 81 ++++--
> include/linux/mfd/amd-sb.h | 32 ++-
> include/uapi/linux/amd-apml.h | 74 ++++++
> 6 files changed, 615 insertions(+), 112 deletions(-)
> create mode 100644 drivers/mfd/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 aaeb5050eb0c..a8bf9aea52f9 100644
> --- a/drivers/hwmon/sbrmi.c
> +++ b/drivers/hwmon/sbrmi.c
> @@ -19,42 +19,46 @@
> 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_sbrmi_device *rmi_dev = dev_get_drvdata(dev);
> + struct apml_message msg = { 0 };
> int ret;
>
> if (type != hwmon_power)
> return -EINVAL;
>
> - msg.read = true;
> + mutex_lock(&rmi_dev->lock);
> + msg.data_in.reg_in[RD_FLAG_INDEX] = 1;
> switch (attr) {
> case hwmon_power_input:
> msg.cmd = SBRMI_READ_PKG_PWR_CONSUMPTION;
> - ret = rmi_mailbox_xfer(data, &msg);
> + ret = rmi_mailbox_xfer(rmi_dev, &msg);
> break;
> case hwmon_power_cap:
> msg.cmd = SBRMI_READ_PKG_PWR_LIMIT;
> - ret = rmi_mailbox_xfer(data, &msg);
> + ret = rmi_mailbox_xfer(rmi_dev, &msg);
> break;
> case hwmon_power_cap_max:
> - msg.data_out = data->pwr_limit_max;
> ret = 0;
> + msg.data_out.mb_out[RD_WR_DATA_INDEX] = rmi_dev->pwr_limit_max;
> 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;
> +
> + mutex_unlock(&rmi_dev->lock);
For both the read and write function, there is only a single call into the rmi
code. I don't see why it would be necessary to move mutex protection out of
rmi_mailbox_xfer() and into the hwmon subsystem. IMO, from a hwmon perspective,
this change is unnecessary. If rmi_mailbox_xfer() needs a protected and an
unprotected version, both should be provided by the rmi core, and the hwmon
driver should continue calling the function which handles the mutex internally.
Thanks,
Guenter
> return ret;
> }
>
> 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_sbrmi_device *rmi_dev = dev_get_drvdata(dev);
> + struct apml_message msg = { 0 };
> + int ret;
>
> if (type != hwmon_power && attr != hwmon_power_cap)
> return -EINVAL;
> @@ -64,13 +68,16 @@ static int sbrmi_write(struct device *dev, enum hwmon_sensor_types type,
> */
> val /= 1000;
>
> - val = clamp_val(val, SBRMI_PWR_MIN, data->pwr_limit_max);
> + val = clamp_val(val, SBRMI_PWR_MIN, rmi_dev->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);
> + mutex_lock(&rmi_dev->lock);
> + ret = rmi_mailbox_xfer(rmi_dev, &msg);
> + mutex_unlock(&rmi_dev->lock);
> + return ret;
> }
>
> static umode_t sbrmi_is_visible(const void *data,
> @@ -114,9 +121,9 @@ static int sbrmi_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct device *hwmon_dev;
> - struct sbrmi_data *data = dev_get_drvdata(pdev->dev.parent);
> + struct apml_sbrmi_device *rmi_dev = dev_get_drvdata(pdev->dev.parent);
>
> - hwmon_dev = devm_hwmon_device_register_with_info(dev, "sbrmi", data,
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "sbrmi", rmi_dev,
> &sbrmi_chip_info, NULL);
> return PTR_ERR_OR_ZERO(hwmon_dev);
> }
> diff --git a/drivers/mfd/sbrmi-core.c b/drivers/mfd/sbrmi-core.c
> index d5fbe5676cab..05d6c68d78b6 100644
> --- a/drivers/mfd/sbrmi-core.c
> +++ b/drivers/mfd/sbrmi-core.c
> @@ -7,45 +7,271 @@
> */
> #include <linux/delay.h>
> #include <linux/err.h>
> +#include <linux/fs.h>
> #include <linux/mfd/amd-sb.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/regmap.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 apml_sbrmi_device *rmi_dev)
> +{
> + 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(rmi_dev->regmap,
> + msg.data_in.reg_in[REG_OFF_INDEX],
> + &msg.data_out.mb_out[RD_WR_DATA_INDEX]);
> + if (ret < 0)
> + return ret;
> +
> + rmi_dev->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 apml_sbrmi_device *rmi_dev,
> + int *status, int mask)
> +{
> + int ret, retry = 100;
> +
> + do {
> + ret = regmap_read(rmi_dev->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 apml_sbrmi_device *rmi_dev,
> + 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 (!rmi_dev->rev) {
> + ret = sbrmi_get_rev(rmi_dev);
> + if (ret < 0)
> + return ret;
> + }
> + /* MCA MSR protocol for REV 0x10 is not supported*/
> + if (rmi_dev->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(rmi_dev->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(rmi_dev->regmap, CPUID_MCA_CMD,
> + &input, MSR_WR_REG_LEN);
> + if (ret < 0)
> + goto exit_unlock;
> +
> + ret = sbrmi_wait_status(rmi_dev, &hw_status, HW_ALERT_MASK);
> + if (ret < 0)
> + goto exit_unlock;
> +
> + ret = regmap_bulk_read(rmi_dev->regmap, CPUID_MCA_CMD,
> + &output, MSR_RD_REG_LEN);
> + if (ret < 0)
> + goto exit_unlock;
> +
> + ret = regmap_write(rmi_dev->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 apml_sbrmi_device *rmi_dev,
> + 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 (!rmi_dev->rev) {
> + ret = sbrmi_get_rev(rmi_dev);
> + if (ret < 0)
> + return ret;
> + }
> + /* CPUID protocol for REV 0x10 is not supported*/
> + if (rmi_dev->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(rmi_dev->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(rmi_dev->regmap, CPUID_MCA_CMD,
> + &input, CPUID_WR_REG_LEN);
> + if (ret < 0)
> + goto exit_unlock;
> +
> + ret = sbrmi_wait_status(rmi_dev, &hw_status, HW_ALERT_MASK);
> + if (ret < 0)
> + goto exit_unlock;
> +
> + ret = regmap_bulk_read(rmi_dev->regmap, CPUID_MCA_CMD,
> + &output, CPUID_RD_REG_LEN);
> + if (ret < 0)
> + goto exit_unlock;
> +
> + ret = regmap_write(rmi_dev->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)
> +static int sbrmi_clear_status_alert(struct apml_sbrmi_device *rmi_dev)
> {
> int sw_status, ret;
>
> - ret = regmap_read(data->regmap, SBRMI_STATUS,
> + ret = regmap_read(rmi_dev->regmap, SBRMI_STATUS,
> &sw_status);
> if (ret < 0)
> return ret;
> @@ -53,31 +279,31 @@ static int sbrmi_clear_status_alert(struct sbrmi_data *data)
> if (!(sw_status & SW_ALERT_MASK))
> return 0;
>
> - return regmap_write(data->regmap, SBRMI_STATUS,
> + return regmap_write(rmi_dev->regmap, SBRMI_STATUS,
> SW_ALERT_MASK);
> }
>
> -int rmi_mailbox_xfer(struct sbrmi_data *data,
> - struct sbrmi_mailbox_msg *msg)
> +int rmi_mailbox_xfer(struct apml_sbrmi_device *rmi_dev,
> + 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);
> + ret = sbrmi_clear_status_alert(rmi_dev);
> if (ret < 0)
> goto exit_unlock;
>
> /* Indicate firmware a command is to be serviced */
> - ret = regmap_write(data->regmap, SBRMI_INBNDMSG7, START_CMD);
> + ret = regmap_write(rmi_dev->regmap, SBRMI_INBNDMSG7, START_CMD);
> if (ret < 0)
> goto exit_unlock;
>
> /* Write the command to SBRMI::InBndMsg_inst0 */
> - ret = regmap_write(data->regmap, SBRMI_INBNDMSG0, msg->cmd);
> + ret = regmap_write(rmi_dev->regmap, SBRMI_INBNDMSG0, msg->cmd);
> if (ret < 0)
> goto exit_unlock;
>
> @@ -86,9 +312,9 @@ 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;
> - ret = regmap_write(data->regmap, SBRMI_INBNDMSG1 + i, byte);
> + for (i = 0; i < MB_DATA_SIZE; i++) {
> + byte = msg->data_in.reg_in[i];
> + ret = regmap_write(rmi_dev->regmap, SBRMI_INBNDMSG1 + i, byte);
> if (ret < 0)
> goto exit_unlock;
> }
> @@ -97,7 +323,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 = regmap_write(data->regmap, SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX);
> + ret = regmap_write(rmi_dev->regmap, SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX);
> if (ret < 0)
> goto exit_unlock;
>
> @@ -106,46 +332,159 @@ 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(rmi_dev, &sw_status, SW_ALERT_MASK);
> + if (ret < 0)
> goto exit_unlock;
> - }
> +
> + ret = regmap_read(rmi_dev->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++) {
> - ret = regmap_read(data->regmap,
> + if (msg->data_in.reg_in[RD_FLAG_INDEX]) {
> + for (i = 0; i < MB_DATA_SIZE; i++) {
> + ret = regmap_read(rmi_dev->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);
> -
> + ret = regmap_write(rmi_dev->regmap, SBRMI_STATUS,
> + SW_ALERT_MASK);
> + if (ec) {
> + ret = -EPROTOTYPE;
> + msg->fw_ret_code = ec;
> + }
> exit_unlock:
> - mutex_unlock(&data->lock);
> return ret;
> }
> 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 apml_sbrmi_device *rmi_dev = container_of(fp->private_data, struct apml_sbrmi_device,
> + sbrmi_misc_dev);
> + if (!rmi_dev)
> + return -ENODEV;
> +
> + /*
> + * If device remove/unbind is called do not allow new transaction
> + */
> + if (atomic_read(&rmi_dev->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;
> +
> + /*
> + * 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(&rmi_dev->lock);
> + /* Verify device unbind/remove is not invoked */
> + if (atomic_read(&rmi_dev->no_new_trans)) {
> + mutex_unlock(&rmi_dev->lock);
> + return -EBUSY;
> + }
> +
> + /* Is this a read/monitor/get request */
> + if (msg.data_in.reg_in[RD_FLAG_INDEX])
> + read = true;
> +
> + /*
> + * Set the in_progress variable to true, to wait for
> + * completion during unbind/remove of driver
> + */
> + atomic_set(&rmi_dev->in_progress, 1);
> +
> + switch (msg.cmd) {
> + case 0 ... 0x999:
> + /* Mailbox protocol */
> + ret = rmi_mailbox_xfer(rmi_dev, &msg);
> + break;
> + case APML_CPUID:
> + ret = rmi_cpuid_read(rmi_dev, &msg);
> + break;
> + case APML_MCA_MSR:
> + /* MCAMSR protocol */
> + ret = rmi_mca_msr_read(rmi_dev, &msg);
> + break;
> + case APML_REG:
> + /* REG R/W */
> + if (read) {
> + ret = regmap_read(rmi_dev->regmap,
> + msg.data_in.reg_in[REG_OFF_INDEX],
> + &msg.data_out.mb_out[RD_WR_DATA_INDEX]);
> + } else {
> + ret = regmap_write(rmi_dev->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(&rmi_dev->no_new_trans))
> + complete(&rmi_dev->misc_fops_done);
> +
> + atomic_set(&rmi_dev->in_progress, 0);
> + mutex_unlock(&rmi_dev->lock);
> +
> + /* 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 apml_sbrmi_device *rmi_dev,
> + struct device *dev)
> +{
> + int ret;
> +
> + rmi_dev->sbrmi_misc_dev.name = devm_kasprintf(dev,
> + GFP_KERNEL,
> + "sbrmi-%x",
> + rmi_dev->dev_static_addr);
> + rmi_dev->sbrmi_misc_dev.minor = MISC_DYNAMIC_MINOR;
> + rmi_dev->sbrmi_misc_dev.fops = &sbrmi_fops;
> + rmi_dev->sbrmi_misc_dev.parent = dev;
> + rmi_dev->sbrmi_misc_dev.nodename = devm_kasprintf(dev,
> + GFP_KERNEL,
> + "sbrmi-%x",
> + rmi_dev->dev_static_addr);
> + rmi_dev->sbrmi_misc_dev.mode = 0600;
> +
> + ret = misc_register(&rmi_dev->sbrmi_misc_dev);
> + if (ret)
> + return ret;
> +
> + dev_info(dev, "register %s device\n", rmi_dev->sbrmi_misc_dev.name);
> + return ret;
> +}
> diff --git a/drivers/mfd/sbrmi-core.h b/drivers/mfd/sbrmi-core.h
> new file mode 100644
> index 000000000000..6339931d4eb3
> --- /dev/null
> +++ b/drivers/mfd/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 apml_sbrmi_device *rmi_dev,
> + struct device *dev);
> +#endif /*_SBRMI_CORE__H_*/
> diff --git a/drivers/mfd/sbrmi-i2c.c b/drivers/mfd/sbrmi-i2c.c
> index bdf15a7a2167..a8dcb7a92af8 100644
> --- a/drivers/mfd/sbrmi-i2c.c
> +++ b/drivers/mfd/sbrmi-i2c.c
> @@ -12,18 +12,18 @@
> #include <linux/init.h>
> #include <linux/mfd/core.h>
> #include <linux/mfd/amd-sb.h>
> -#include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/of.h>
> #include <linux/regmap.h>
> +#include "sbrmi-core.h"
>
> -#define SBRMI_CTRL 0x1
> +#define MAX_WAIT_TIME_SEC (3)
>
> static struct mfd_cell apml_sbrmi[] = {
> { .name = "sbrmi-hwmon" },
> };
>
> -static int sbrmi_enable_alert(struct sbrmi_data *data)
> +static int sbrmi_enable_alert(struct apml_sbrmi_device *rmi_dev)
> {
> int ctrl, ret;
>
> @@ -31,29 +31,29 @@ static int sbrmi_enable_alert(struct sbrmi_data *data)
> * Enable the SB-RMI Software alert status
> * by writing 0 to bit 4 of Control register(0x1)
> */
> - ret = regmap_read(data->regmap, SBRMI_CTRL, &ctrl);
> + ret = regmap_read(rmi_dev->regmap, SBRMI_CTRL, &ctrl);
> if (ret < 0)
> return ret;
>
> if (ctrl & 0x10) {
> ctrl &= ~0x10;
> - return regmap_write(data->regmap, SBRMI_CTRL, ctrl);
> + return regmap_write(rmi_dev->regmap, SBRMI_CTRL, ctrl);
> }
>
> return 0;
> }
>
> -static int sbrmi_get_max_pwr_limit(struct sbrmi_data *data)
> +static int sbrmi_get_max_pwr_limit(struct apml_sbrmi_device *rmi_dev)
> {
> - struct sbrmi_mailbox_msg msg = { 0 };
> + struct apml_message msg = { 0 };
> int ret;
>
> msg.cmd = SBRMI_READ_PKG_MAX_PWR_LIMIT;
> - msg.read = true;
> - ret = rmi_mailbox_xfer(data, &msg);
> + msg.data_in.reg_in[RD_FLAG_INDEX] = 1;
> + ret = rmi_mailbox_xfer(rmi_dev, &msg);
> if (ret < 0)
> return ret;
> - data->pwr_limit_max = msg.data_out;
> + rmi_dev->pwr_limit_max = msg.data_out.mb_out[RD_WR_DATA_INDEX];
>
> return ret;
> }
> @@ -61,40 +61,76 @@ static int sbrmi_get_max_pwr_limit(struct sbrmi_data *data)
> static int sbrmi_i2c_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> - struct sbrmi_data *data;
> + struct apml_sbrmi_device *rmi_dev;
> 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)
> + rmi_dev = devm_kzalloc(dev, sizeof(struct apml_sbrmi_device), GFP_KERNEL);
> + if (!rmi_dev)
> return -ENOMEM;
>
> - mutex_init(&data->lock);
> + atomic_set(&rmi_dev->in_progress, 0);
> + atomic_set(&rmi_dev->no_new_trans, 0);
> + mutex_init(&rmi_dev->lock);
>
> - data->regmap = devm_regmap_init_i2c(client, &sbrmi_i2c_regmap_config);
> - if (IS_ERR(data->regmap))
> - return PTR_ERR(data->regmap);
> + rmi_dev->regmap = devm_regmap_init_i2c(client, &sbrmi_i2c_regmap_config);
> + if (IS_ERR(rmi_dev->regmap))
> + return PTR_ERR(rmi_dev->regmap);
>
> /* Enable alert for SB-RMI sequence */
> - ret = sbrmi_enable_alert(data);
> + ret = sbrmi_enable_alert(rmi_dev);
> if (ret < 0)
> return ret;
>
> /* Cache maximum power limit */
> - ret = sbrmi_get_max_pwr_limit(data);
> + ret = sbrmi_get_max_pwr_limit(rmi_dev);
> if (ret < 0)
> return ret;
>
> - dev_set_drvdata(dev, (void *)data);
> + dev_set_drvdata(dev, (void *)rmi_dev);
>
> ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, apml_sbrmi,
> ARRAY_SIZE(apml_sbrmi), NULL, 0, NULL);
> - if (ret)
> + if (ret) {
> dev_err(dev, "Failed to register for sub-devices: %d\n", ret);
> - return ret;
> + return ret;
> + }
> +
> + rmi_dev->dev_static_addr = client->addr;
> + init_completion(&rmi_dev->misc_fops_done);
> + return create_misc_rmi_device(rmi_dev, dev);
> +}
> +
> +static void sbrmi_i2c_remove(struct i2c_client *client)
> +{
> + struct apml_sbrmi_device *rmi_dev = dev_get_drvdata(&client->dev);
> +
> + if (!rmi_dev)
> + return;
> + /*
> + * Set the no_new_trans so no new transaction can
> + * occur in sbrmi_ioctl
> + */
> + atomic_set(&rmi_dev->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(&rmi_dev->in_progress))
> + wait_for_completion_timeout(&rmi_dev->misc_fops_done,
> + MAX_WAIT_TIME_SEC * HZ);
> + misc_deregister(&rmi_dev->sbrmi_misc_dev);
> + /* Assign fops and parent of misc dev to NULL */
> + rmi_dev->sbrmi_misc_dev.fops = NULL;
> + rmi_dev->sbrmi_misc_dev.parent = NULL;
> + dev_info(&client->dev, "Removed sbrmi driver\n");
> +
> + return;
> }
>
> static const struct i2c_device_id sbrmi_id[] = {
> @@ -117,6 +153,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/linux/mfd/amd-sb.h b/include/linux/mfd/amd-sb.h
> index 977b8228ffa1..7bb5b89bb71a 100644
> --- a/include/linux/mfd/amd-sb.h
> +++ b/include/linux/mfd/amd-sb.h
> @@ -6,8 +6,10 @@
> #ifndef _AMD_SB_H_
> #define _AMD_SB_H_
>
> +#include <linux/miscdevice.h>
> #include <linux/mutex.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.
> @@ -20,24 +22,32 @@ enum sbrmi_msg_id {
> SBRMI_READ_PKG_MAX_PWR_LIMIT,
> };
>
> -/* Each client has this additional data */
> -struct sbrmi_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 apml_sbrmi_device {
> + struct completion misc_fops_done;
> + struct miscdevice sbrmi_misc_dev;
> struct regmap *regmap;
> + /* Mutex locking */
> struct mutex lock;
> + 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_MFD_SBRMI_I2C)
> -int rmi_mailbox_xfer(struct sbrmi_data *data, struct sbrmi_mailbox_msg *msg);
> +int rmi_mailbox_xfer(struct apml_sbrmi_device *rmi_dev,
> + struct apml_message *msg);
> #else
> -int rmi_mailbox_xfer(struct sbrmi_data *data, struct sbrmi_mailbox_msg *msg)
> +int rmi_mailbox_xfer(struct apml_sbrmi_device *rmi_dev,
> + 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_*/
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] mfd: add amd side-band functionality
2024-05-30 11:23 [PATCH 0/5] mfd: add amd side-band functionality Naveen Krishna Chatradhi
` (4 preceding siblings ...)
2024-05-30 11:23 ` [PATCH 5/5] mfd/hwmon: sbrmi: Add support for APML protocols Naveen Krishna Chatradhi
@ 2024-06-13 17:05 ` Lee Jones
2024-06-14 13:56 ` Chatradhi, Naveen Krishna
5 siblings, 1 reply; 15+ messages in thread
From: Lee Jones @ 2024-06-13 17:05 UTC (permalink / raw)
To: Naveen Krishna Chatradhi
Cc: linux-hwmon, inux-kernel, linux, gregkh, arnd, Akshay Gupta
On Thu, 30 May 2024, Naveen Krishna Chatradhi wrote:
> From: Akshay Gupta <akshay.gupta@amd.com>
>
> At present, sbrmi under hwmon subsystem is probed as an i2c
> driver and reports power.
>
> However, APML interface defines few other protocols to support
> OOB system management functionality.
>
> This patchset the following
> 1. Based on hwmon maintainers feedback, move the i2c client
> probe and sbrmi core functionality to drivers/mfd/
> 2. Add an MFD cell, which probes the hwmon/sbrmi and continues to
> report power using the symbol exported by the mfd/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 for the following
> - Mailbox xfer (already defined in sbrmi_mailbox_xfer())
> - CPUID access
> - MCAMSR access
>
> Akshay Gupta (5):
> hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD
> mfd: sbrmi: Add mfd cell to I2C probe to be used by hwmon
> mfd/hwmon sbrmi: Use regmap subsystem
> mfd: sbrmi: Clear sbrmi status register bit SwAlertSts
> mfd/hwmon: sbrmi: Add support for APML protocols
>
> drivers/hwmon/Kconfig | 1 +
> drivers/hwmon/sbrmi.c | 284 +++-----------------
> drivers/mfd/Kconfig | 9 +-
> drivers/mfd/Makefile | 2 +
> drivers/mfd/sbrmi-core.c | 490 ++++++++++++++++++++++++++++++++++
It's not clear to me what any of these 500 lines do, but they do not
look like a good fit for MFD. Perhaps I'm missing something. Can you
provide some more information about the device and why you think MFD is
a suitable location for it?
> drivers/mfd/sbrmi-core.h | 37 +++
> drivers/mfd/sbrmi-i2c.c | 165 ++++++++++++
> include/linux/mfd/amd-sb.h | 55 ++++
> include/uapi/linux/amd-apml.h | 74 +++++
> 9 files changed, 871 insertions(+), 246 deletions(-)
> create mode 100644 drivers/mfd/sbrmi-core.c
> create mode 100644 drivers/mfd/sbrmi-core.h
> create mode 100644 drivers/mfd/sbrmi-i2c.c
> create mode 100644 include/linux/mfd/amd-sb.h
> create mode 100644 include/uapi/linux/amd-apml.h
>
> --
> 2.25.1
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/5] mfd: add amd side-band functionality
2024-06-13 17:05 ` [PATCH 0/5] mfd: add amd side-band functionality Lee Jones
@ 2024-06-14 13:56 ` Chatradhi, Naveen Krishna
2024-06-14 14:49 ` Lee Jones
0 siblings, 1 reply; 15+ messages in thread
From: Chatradhi, Naveen Krishna @ 2024-06-14 13:56 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-hwmon, inux-kernel, linux, gregkh, arnd, Akshay Gupta
On 6/13/2024 10:35 PM, Lee Jones wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, 30 May 2024, Naveen Krishna Chatradhi wrote:
>
>> From: Akshay Gupta <akshay.gupta@amd.com>
>>
>> At present, sbrmi under hwmon subsystem is probed as an i2c
>> driver and reports power.
>>
>> However, APML interface defines few other protocols to support
>> OOB system management functionality.
>>
>> This patchset the following
>> 1. Based on hwmon maintainers feedback, move the i2c client
>> probe and sbrmi core functionality to drivers/mfd/
>> 2. Add an MFD cell, which probes the hwmon/sbrmi and continues to
>> report power using the symbol exported by the mfd/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 for the following
>> - Mailbox xfer (already defined in sbrmi_mailbox_xfer())
>> - CPUID access
>> - MCAMSR access
>>
>> Akshay Gupta (5):
>> hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD
>> mfd: sbrmi: Add mfd cell to I2C probe to be used by hwmon
>> mfd/hwmon sbrmi: Use regmap subsystem
>> mfd: sbrmi: Clear sbrmi status register bit SwAlertSts
>> mfd/hwmon: sbrmi: Add support for APML protocols
>>
>> drivers/hwmon/Kconfig | 1 +
>> drivers/hwmon/sbrmi.c | 284 +++-----------------
>> drivers/mfd/Kconfig | 9 +-
>> drivers/mfd/Makefile | 2 +
>> drivers/mfd/sbrmi-core.c | 490 ++++++++++++++++++++++++++++++++++
> It's not clear to me what any of these 500 lines do, but they do not
> look like a good fit for MFD. Perhaps I'm missing something. Can you
> provide some more information about the device and why you think MFD is
> a suitable location for it?
Hi lee,
Data center processors from AMD provide a side-band (often called
out-of-band) path for manageability
called Advanced Platform Management Link (APML), which consists of
i2c/i3c client devices called
Side-band Remote Management Interface (SB-RMI) and Side-band Temperature
Sensor Interface (SB-TSI).
We have i2c client drivers upstreamed under drivers/hwmon sbrmi.c and
sbtsi_temp.c reporting power and
temperature via hwmon interfaces. However, sbrmi device can also provide
performance, telemetry and RAS
monitoring, management using AMD defined protocols. One of them
sbrmi_mailbox_xfer()is defined in
drivers/hwmon/sbrmi.c.
Patchset would do the following
1. Move core functionality from hwmon/sbrmi.c to drivers/mfd/sbrmi-i2c.c
as an i2c_driver.
2. Convert the hwmon/sbrmi.c to a platform driver.
3. Use mfd_add_devices() API to add cells which will probe the platform
driver under hwmon/sbrmi.c
4. drivers/mfd/sbrmi-core.c will contain the common functions which can
be used by i2c and i3c based drivers, such as
core protocol definitions, creation of misc device and an ioctl for the
user interface.
This patchset is an attempt toadd support for these core protocols in such a way that Open-sourced and
widely used https://github.com/amd/esmi_oob_library will continue to
provide user-space programmable API. regards, Naveenk
>> drivers/mfd/sbrmi-core.h | 37 +++
>> drivers/mfd/sbrmi-i2c.c | 165 ++++++++++++
>> include/linux/mfd/amd-sb.h | 55 ++++
>> include/uapi/linux/amd-apml.h | 74 +++++
>> 9 files changed, 871 insertions(+), 246 deletions(-)
>> create mode 100644 drivers/mfd/sbrmi-core.c
>> create mode 100644 drivers/mfd/sbrmi-core.h
>> create mode 100644 drivers/mfd/sbrmi-i2c.c
>> create mode 100644 include/linux/mfd/amd-sb.h
>> create mode 100644 include/uapi/linux/amd-apml.h
>>
>> --
>> 2.25.1
>>
> --
> Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] mfd: add amd side-band functionality
2024-06-14 13:56 ` Chatradhi, Naveen Krishna
@ 2024-06-14 14:49 ` Lee Jones
2024-06-18 7:17 ` Chatradhi, Naveen Krishna
0 siblings, 1 reply; 15+ messages in thread
From: Lee Jones @ 2024-06-14 14:49 UTC (permalink / raw)
To: Chatradhi, Naveen Krishna
Cc: linux-hwmon, inux-kernel, linux, gregkh, arnd, Akshay Gupta
On Fri, 14 Jun 2024, Chatradhi, Naveen Krishna wrote:
>
> On 6/13/2024 10:35 PM, Lee Jones wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> >
> >
> > On Thu, 30 May 2024, Naveen Krishna Chatradhi wrote:
> >
> > > From: Akshay Gupta <akshay.gupta@amd.com>
> > >
> > > At present, sbrmi under hwmon subsystem is probed as an i2c
> > > driver and reports power.
> > >
> > > However, APML interface defines few other protocols to support
> > > OOB system management functionality.
> > >
> > > This patchset the following
> > > 1. Based on hwmon maintainers feedback, move the i2c client
> > > probe and sbrmi core functionality to drivers/mfd/
> > > 2. Add an MFD cell, which probes the hwmon/sbrmi and continues to
> > > report power using the symbol exported by the mfd/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 for the following
> > > - Mailbox xfer (already defined in sbrmi_mailbox_xfer())
> > > - CPUID access
> > > - MCAMSR access
> > >
> > > Akshay Gupta (5):
> > > hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD
> > > mfd: sbrmi: Add mfd cell to I2C probe to be used by hwmon
> > > mfd/hwmon sbrmi: Use regmap subsystem
> > > mfd: sbrmi: Clear sbrmi status register bit SwAlertSts
> > > mfd/hwmon: sbrmi: Add support for APML protocols
> > >
> > > drivers/hwmon/Kconfig | 1 +
> > > drivers/hwmon/sbrmi.c | 284 +++-----------------
> > > drivers/mfd/Kconfig | 9 +-
> > > drivers/mfd/Makefile | 2 +
> > > drivers/mfd/sbrmi-core.c | 490 ++++++++++++++++++++++++++++++++++
> > It's not clear to me what any of these 500 lines do, but they do not
> > look like a good fit for MFD. Perhaps I'm missing something. Can you
> > provide some more information about the device and why you think MFD is
> > a suitable location for it?
>
> Hi lee,
>
> Data center processors from AMD provide a side-band (often called
> out-of-band) path for manageability
>
> called Advanced Platform Management Link (APML), which consists of i2c/i3c
> client devices called
>
> Side-band Remote Management Interface (SB-RMI) and Side-band Temperature
> Sensor Interface (SB-TSI).
>
>
> We have i2c client drivers upstreamed under drivers/hwmon sbrmi.c and
> sbtsi_temp.c reporting power and
>
> temperature via hwmon interfaces. However, sbrmi device can also provide
> performance, telemetry and RAS
MFD knows nothing of these characteristics.
> monitoring, management using AMD defined protocols. One of them
> sbrmi_mailbox_xfer()is defined in
I large portion of this should be moved out to drivers/mailbox.
> drivers/hwmon/sbrmi.c.
>
> Patchset would do the following
>
> 1. Move core functionality from hwmon/sbrmi.c to drivers/mfd/sbrmi-i2c.c as
> an i2c_driver.
>
> 2. Convert the hwmon/sbrmi.c to a platform driver.
>
> 3. Use mfd_add_devices() API to add cells which will probe the platform
> driver under hwmon/sbrmi.c
How many devices will mfd_add_devices() be registering?
> 4. drivers/mfd/sbrmi-core.c will contain the common functions which can be
> used by i2c and i3c based drivers, such as
>
> core protocol definitions, creation of misc device and an ioctl for the user
drivers/misc?
> interface.
>
> This patchset is an attempt toadd support for these core protocols in such a
> way that Open-sourced and widely used
> https://github.com/amd/esmi_oob_library will continue to provide user-space
> programmable API. regards, Naveenk
>
> > > drivers/mfd/sbrmi-core.h | 37 +++
> > > drivers/mfd/sbrmi-i2c.c | 165 ++++++++++++
> > > include/linux/mfd/amd-sb.h | 55 ++++
> > > include/uapi/linux/amd-apml.h | 74 +++++
> > > 9 files changed, 871 insertions(+), 246 deletions(-)
> > > create mode 100644 drivers/mfd/sbrmi-core.c
> > > create mode 100644 drivers/mfd/sbrmi-core.h
> > > create mode 100644 drivers/mfd/sbrmi-i2c.c
> > > create mode 100644 include/linux/mfd/amd-sb.h
> > > create mode 100644 include/uapi/linux/amd-apml.h
> > >
> > > --
> > > 2.25.1
> > >
> > --
> > Lee Jones [李琼斯]
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] mfd: add amd side-band functionality
2024-06-14 14:49 ` Lee Jones
@ 2024-06-18 7:17 ` Chatradhi, Naveen Krishna
2024-06-18 12:27 ` Lee Jones
0 siblings, 1 reply; 15+ messages in thread
From: Chatradhi, Naveen Krishna @ 2024-06-18 7:17 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-hwmon, inux-kernel, linux, gregkh, arnd, Akshay Gupta
On 6/14/2024 8:19 PM, Lee Jones wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Fri, 14 Jun 2024, Chatradhi, Naveen Krishna wrote:
>
>> On 6/13/2024 10:35 PM, Lee Jones wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> On Thu, 30 May 2024, Naveen Krishna Chatradhi wrote:
>>>
>>>> From: Akshay Gupta <akshay.gupta@amd.com>
>>>>
>>>> At present, sbrmi under hwmon subsystem is probed as an i2c
>>>> driver and reports power.
>>>>
>>>> However, APML interface defines few other protocols to support
>>>> OOB system management functionality.
>>>>
>>>> This patchset the following
>>>> 1. Based on hwmon maintainers feedback, move the i2c client
>>>> probe and sbrmi core functionality to drivers/mfd/
>>>> 2. Add an MFD cell, which probes the hwmon/sbrmi and continues to
>>>> report power using the symbol exported by the mfd/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 for the following
>>>> - Mailbox xfer (already defined in sbrmi_mailbox_xfer())
>>>> - CPUID access
>>>> - MCAMSR access
>>>>
>>>> Akshay Gupta (5):
>>>> hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD
>>>> mfd: sbrmi: Add mfd cell to I2C probe to be used by hwmon
>>>> mfd/hwmon sbrmi: Use regmap subsystem
>>>> mfd: sbrmi: Clear sbrmi status register bit SwAlertSts
>>>> mfd/hwmon: sbrmi: Add support for APML protocols
>>>>
>>>> drivers/hwmon/Kconfig | 1 +
>>>> drivers/hwmon/sbrmi.c | 284 +++-----------------
>>>> drivers/mfd/Kconfig | 9 +-
>>>> drivers/mfd/Makefile | 2 +
>>>> drivers/mfd/sbrmi-core.c | 490 ++++++++++++++++++++++++++++++++++
>>> It's not clear to me what any of these 500 lines do, but they do not
>>> look like a good fit for MFD. Perhaps I'm missing something. Can you
>>> provide some more information about the device and why you think MFD is
>>> a suitable location for it?
>> Hi lee,
>>
>> Data center processors from AMD provide a side-band (often called
>> out-of-band) path for manageability
>>
>> called Advanced Platform Management Link (APML), which consists of i2c/i3c
>> client devices called
>>
>> Side-band Remote Management Interface (SB-RMI) and Side-band Temperature
>> Sensor Interface (SB-TSI).
>>
>>
>> We have i2c client drivers upstreamed under drivers/hwmon sbrmi.c and
>> sbtsi_temp.c reporting power and
>>
>> temperature via hwmon interfaces. However, sbrmi device can also provide
>> performance, telemetry and RAS
> MFD knows nothing of these characteristics.
Yes, we will modify the implementation to define ops structure with
functionality that
can be called by platforms drivers in hwmon and other subsystems.
>
>> monitoring, management using AMD defined protocols. One of them
>> sbrmi_mailbox_xfer()is defined in
> I large portion of this should be moved out to drivers/mailbox.
we have explored the mailbox subsystem, APML xfer is not a ful-fledge
mailbox interface as such,
it is a custom protocol, which accepts inputs and provides outputs over
i2c/i3c. It does not support
multichannel (tx/rx) or have IRQs or a memory mapped IO and it is one of
the protocols supported
by the RMI device.
>
>> drivers/hwmon/sbrmi.c.
>>
>> Patchset would do the following
>>
>> 1. Move core functionality from hwmon/sbrmi.c to drivers/mfd/sbrmi-i2c.c as
>> an i2c_driver.
>>
>> 2. Convert the hwmon/sbrmi.c to a platform driver.
>>
>> 3. Use mfd_add_devices() API to add cells which will probe the platform
>> driver under hwmon/sbrmi.c
> How many devices will mfd_add_devices() be registering?
This patch is adding one hwmon device.
We can add additional cell which probes a platform driver in
drivers/misc which handles
the user space interface part.
>
>> 4. drivers/mfd/sbrmi-core.c will contain the common functions which can be
>> used by i2c and i3c based drivers, such as
>>
>> core protocol definitions, creation of misc device and an ioctl for the user
> drivers/misc?
We will move the misc device registration and user space interface to
drivers/misc and
use the ops provided by this mfd device.
Regards,
naveenk
>
>> interface.
>>
>> This patchset is an attempt toadd support for these core protocols in such a
>> way that Open-sourced and widely used
>> https://github.com/amd/esmi_oob_library will continue to provide user-space
>> programmable API. regards, Naveenk
>>
>>>> drivers/mfd/sbrmi-core.h | 37 +++
>>>> drivers/mfd/sbrmi-i2c.c | 165 ++++++++++++
>>>> include/linux/mfd/amd-sb.h | 55 ++++
>>>> include/uapi/linux/amd-apml.h | 74 +++++
>>>> 9 files changed, 871 insertions(+), 246 deletions(-)
>>>> create mode 100644 drivers/mfd/sbrmi-core.c
>>>> create mode 100644 drivers/mfd/sbrmi-core.h
>>>> create mode 100644 drivers/mfd/sbrmi-i2c.c
>>>> create mode 100644 include/linux/mfd/amd-sb.h
>>>> create mode 100644 include/uapi/linux/amd-apml.h
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>> --
>>> Lee Jones [李琼斯]
> --
> Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] mfd: add amd side-band functionality
2024-06-18 7:17 ` Chatradhi, Naveen Krishna
@ 2024-06-18 12:27 ` Lee Jones
2024-06-28 11:56 ` Chatradhi, Naveen Krishna
0 siblings, 1 reply; 15+ messages in thread
From: Lee Jones @ 2024-06-18 12:27 UTC (permalink / raw)
To: Chatradhi, Naveen Krishna
Cc: linux-hwmon, inux-kernel, linux, gregkh, arnd, Akshay Gupta
On Tue, 18 Jun 2024, Chatradhi, Naveen Krishna wrote:
>
> On 6/14/2024 8:19 PM, Lee Jones wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> >
> >
> > On Fri, 14 Jun 2024, Chatradhi, Naveen Krishna wrote:
> >
> > > On 6/13/2024 10:35 PM, Lee Jones wrote:
> > > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > > >
> > > >
> > > > On Thu, 30 May 2024, Naveen Krishna Chatradhi wrote:
> > > >
> > > > > From: Akshay Gupta <akshay.gupta@amd.com>
> > > > >
> > > > > At present, sbrmi under hwmon subsystem is probed as an i2c
> > > > > driver and reports power.
> > > > >
> > > > > However, APML interface defines few other protocols to support
> > > > > OOB system management functionality.
> > > > >
> > > > > This patchset the following
> > > > > 1. Based on hwmon maintainers feedback, move the i2c client
> > > > > probe and sbrmi core functionality to drivers/mfd/
> > > > > 2. Add an MFD cell, which probes the hwmon/sbrmi and continues to
> > > > > report power using the symbol exported by the mfd/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 for the following
> > > > > - Mailbox xfer (already defined in sbrmi_mailbox_xfer())
> > > > > - CPUID access
> > > > > - MCAMSR access
> > > > >
> > > > > Akshay Gupta (5):
> > > > > hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD
> > > > > mfd: sbrmi: Add mfd cell to I2C probe to be used by hwmon
> > > > > mfd/hwmon sbrmi: Use regmap subsystem
> > > > > mfd: sbrmi: Clear sbrmi status register bit SwAlertSts
> > > > > mfd/hwmon: sbrmi: Add support for APML protocols
> > > > >
> > > > > drivers/hwmon/Kconfig | 1 +
> > > > > drivers/hwmon/sbrmi.c | 284 +++-----------------
> > > > > drivers/mfd/Kconfig | 9 +-
> > > > > drivers/mfd/Makefile | 2 +
> > > > > drivers/mfd/sbrmi-core.c | 490 ++++++++++++++++++++++++++++++++++
> > > > It's not clear to me what any of these 500 lines do, but they do not
> > > > look like a good fit for MFD. Perhaps I'm missing something. Can you
> > > > provide some more information about the device and why you think MFD is
> > > > a suitable location for it?
> > > Hi lee,
> > >
> > > Data center processors from AMD provide a side-band (often called
> > > out-of-band) path for manageability
> > >
> > > called Advanced Platform Management Link (APML), which consists of i2c/i3c
> > > client devices called
> > >
> > > Side-band Remote Management Interface (SB-RMI) and Side-band Temperature
> > > Sensor Interface (SB-TSI).
> > >
> > >
> > > We have i2c client drivers upstreamed under drivers/hwmon sbrmi.c and
> > > sbtsi_temp.c reporting power and
> > >
> > > temperature via hwmon interfaces. However, sbrmi device can also provide
> > > performance, telemetry and RAS
> > MFD knows nothing of these characteristics.
>
> Yes, we will modify the implementation to define ops structure with
> functionality that
>
> can be called by platforms drivers in hwmon and other subsystems.
>
> >
> > > monitoring, management using AMD defined protocols. One of them
> > > sbrmi_mailbox_xfer()is defined in
> > I large portion of this should be moved out to drivers/mailbox.
>
> we have explored the mailbox subsystem, APML xfer is not a ful-fledge
> mailbox interface as such,
>
> it is a custom protocol, which accepts inputs and provides outputs over
> i2c/i3c. It does not support
>
> multichannel (tx/rx) or have IRQs or a memory mapped IO and it is one of the
> protocols supported
>
> by the RMI device.
>
> >
> > > drivers/hwmon/sbrmi.c.
> > >
> > > Patchset would do the following
> > >
> > > 1. Move core functionality from hwmon/sbrmi.c to drivers/mfd/sbrmi-i2c.c as
> > > an i2c_driver.
> > >
> > > 2. Convert the hwmon/sbrmi.c to a platform driver.
> > >
> > > 3. Use mfd_add_devices() API to add cells which will probe the platform
> > > driver under hwmon/sbrmi.c
> > How many devices will mfd_add_devices() be registering?
>
> This patch is adding one hwmon device.
>
> We can add additional cell which probes a platform driver in drivers/misc
> which handles
>
> the user space interface part.
It sounds like MFD is (once more) being used as a dumping ground for
random devices which do not fit anywhere else. A Multi-Function Device
driver's role is to create shared resources (memory, IRQs, Clocks,
Regulators, etc) for and register more than one real device that uses
those shared resources, that's all. Even if you were to move the Misc
part out, using that as a second device to prove it MFD-worthy is not
going to fly. Take a look at what these devices usually consist of:
git grep -W "struct mfd_cell.*{" -- drivers/mfd
This submission results in a 650-line driver that registers a single
cell. One that is attributed only to the device it's being removed
from.
I see that Guenter already said:
"This is not hardware monitoring functionality and would have to
reside elsewhere, outside the hwmon subsystem."
Well it's not MFD functionality either. Sorry.
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/5] mfd: add amd side-band functionality
2024-06-18 12:27 ` Lee Jones
@ 2024-06-28 11:56 ` Chatradhi, Naveen Krishna
0 siblings, 0 replies; 15+ messages in thread
From: Chatradhi, Naveen Krishna @ 2024-06-28 11:56 UTC (permalink / raw)
To: Lee Jones, gregkh, arnd; +Cc: linux-hwmon, inux-kernel, linux, Akshay Gupta
On 6/18/2024 5:57 PM, Lee Jones wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Tue, 18 Jun 2024, Chatradhi, Naveen Krishna wrote:
>
>> On 6/14/2024 8:19 PM, Lee Jones wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> On Fri, 14 Jun 2024, Chatradhi, Naveen Krishna wrote:
>>>
>>>> On 6/13/2024 10:35 PM, Lee Jones wrote:
>>>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>>>
>>>>>
>>>>> On Thu, 30 May 2024, Naveen Krishna Chatradhi wrote:
>>>>>
>>>>>> From: Akshay Gupta <akshay.gupta@amd.com>
>>>>>>
>>>>>> At present, sbrmi under hwmon subsystem is probed as an i2c
>>>>>> driver and reports power.
>>>>>>
>>>>>> However, APML interface defines few other protocols to support
>>>>>> OOB system management functionality.
>>>>>>
>>>>>> This patchset the following
>>>>>> 1. Based on hwmon maintainers feedback, move the i2c client
>>>>>> probe and sbrmi core functionality to drivers/mfd/
>>>>>> 2. Add an MFD cell, which probes the hwmon/sbrmi and continues to
>>>>>> report power using the symbol exported by the mfd/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 for the following
>>>>>> - Mailbox xfer (already defined in sbrmi_mailbox_xfer())
>>>>>> - CPUID access
>>>>>> - MCAMSR access
>>>>>>
>>>>>> Akshay Gupta (5):
>>>>>> hwmon/mfd sbrmi: Move core sbrmi from hwmon to MFD
>>>>>> mfd: sbrmi: Add mfd cell to I2C probe to be used by hwmon
>>>>>> mfd/hwmon sbrmi: Use regmap subsystem
>>>>>> mfd: sbrmi: Clear sbrmi status register bit SwAlertSts
>>>>>> mfd/hwmon: sbrmi: Add support for APML protocols
>>>>>>
>>>>>> drivers/hwmon/Kconfig | 1 +
>>>>>> drivers/hwmon/sbrmi.c | 284 +++-----------------
>>>>>> drivers/mfd/Kconfig | 9 +-
>>>>>> drivers/mfd/Makefile | 2 +
>>>>>> drivers/mfd/sbrmi-core.c | 490 ++++++++++++++++++++++++++++++++++
>>>>> It's not clear to me what any of these 500 lines do, but they do not
>>>>> look like a good fit for MFD. Perhaps I'm missing something. Can you
>>>>> provide some more information about the device and why you think MFD is
>>>>> a suitable location for it?
>>>> Hi lee,
>>>>
>>>> Data center processors from AMD provide a side-band (often called
>>>> out-of-band) path for manageability
>>>>
>>>> called Advanced Platform Management Link (APML), which consists of i2c/i3c
>>>> client devices called
>>>>
>>>> Side-band Remote Management Interface (SB-RMI) and Side-band Temperature
>>>> Sensor Interface (SB-TSI).
>>>>
>>>>
>>>> We have i2c client drivers upstreamed under drivers/hwmon sbrmi.c and
>>>> sbtsi_temp.c reporting power and
>>>>
>>>> temperature via hwmon interfaces. However, sbrmi device can also provide
>>>> performance, telemetry and RAS
>>> MFD knows nothing of these characteristics.
>> Yes, we will modify the implementation to define ops structure with
>> functionality that
>>
>> can be called by platforms drivers in hwmon and other subsystems.
>>
>>>> monitoring, management using AMD defined protocols. One of them
>>>> sbrmi_mailbox_xfer()is defined in
>>> I large portion of this should be moved out to drivers/mailbox.
>> we have explored the mailbox subsystem, APML xfer is not a ful-fledge
>> mailbox interface as such,
>>
>> it is a custom protocol, which accepts inputs and provides outputs over
>> i2c/i3c. It does not support
>>
>> multichannel (tx/rx) or have IRQs or a memory mapped IO and it is one of the
>> protocols supported
>>
>> by the RMI device.
>>
>>>> drivers/hwmon/sbrmi.c.
>>>>
>>>> Patchset would do the following
>>>>
>>>> 1. Move core functionality from hwmon/sbrmi.c to drivers/mfd/sbrmi-i2c.c as
>>>> an i2c_driver.
>>>>
>>>> 2. Convert the hwmon/sbrmi.c to a platform driver.
>>>>
>>>> 3. Use mfd_add_devices() API to add cells which will probe the platform
>>>> driver under hwmon/sbrmi.c
>>> How many devices will mfd_add_devices() be registering?
>> This patch is adding one hwmon device.
>>
>> We can add additional cell which probes a platform driver in drivers/misc
>> which handles
>>
>> the user space interface part.
> It sounds like MFD is (once more) being used as a dumping ground for
> random devices which do not fit anywhere else. A Multi-Function Device
> driver's role is to create shared resources (memory, IRQs, Clocks,
> Regulators, etc) for and register more than one real device that uses
> those shared resources, that's all. Even if you were to move the Misc
> part out, using that as a second device to prove it MFD-worthy is not
> going to fly. Take a look at what these devices usually consist of:
>
> git grep -W "struct mfd_cell.*{" -- drivers/mfd
sbrmi is one physical i2c/i3c device with a set of registers and an IRQ
which provides multiple functions via different protocols.
|------------| -> apml_xfer
| sbrmi | -> cpuid_xfer
|-----------| -> msr_mca_xfer
L-----------> IRQ
We were thinking of adding each functionality as an mfd_cell + 1
mfd_cell for hwmon interface.
I understand it now, it doesn't fit well. Can you suggest any other
sub-system that can absorb this.
I can think of moving everything to a misc driver with a misc device
node and
register a hwmon device for reporting power. please suggest.
>
> This submission results in a 650-line driver that registers a single
> cell. One that is attributed only to the device it's being removed
> from.
>
> I see that Guenter already said:
>
> "This is not hardware monitoring functionality and would have to
> reside elsewhere, outside the hwmon subsystem."
>
> Well it's not MFD functionality either. Sorry.
>
> --
> Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 15+ messages in thread