The existing mail mechanism only supports writing of workload types.
But mailbox command for RFIM (cmd = 0x08) also requires write operation
which was ignored. This results in failing to store RFI restriction.
This requires enhancing mailbox writes for non workload commands also.
So, remove the check for MBOX_CMD_WORKLOAD_TYPE_WRITE in mailbox write,
with this other write commands also can be supoorted. But at the same
time, we have to make sure that there is no impact on read commands,
by not writing anything in mailbox data register.
To properly implement, add two separate functions for mbox read and write
command for processor thermal workload command type. This helps to
differentiate the read and write workload command types while sending mbox
command.
Fixes: 5d6fbc96bd36 ("thermal/drivers/int340x: processor_thermal: Export additional attributes")
Signed-off-by: Sumeet Pawnikar <[email protected]>
Cc: [email protected] # 5.14+
---
.../processor_thermal_device.h | 3 +-
.../int340x_thermal/processor_thermal_mbox.c | 101 +++++++++++-------
.../int340x_thermal/processor_thermal_rfim.c | 23 ++--
3 files changed, 74 insertions(+), 53 deletions(-)
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
index be27f633e40a..43def8c5d2ce 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
@@ -80,7 +80,8 @@ void proc_thermal_rfim_remove(struct pci_dev *pdev);
int proc_thermal_mbox_add(struct pci_dev *pdev, struct proc_thermal_device *proc_priv);
void proc_thermal_mbox_remove(struct pci_dev *pdev);
-int processor_thermal_send_mbox_cmd(struct pci_dev *pdev, u16 cmd_id, u32 cmd_data, u64 *cmd_resp);
+int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u32 data, u64 *resp);
+int processor_thermal_send_mbox_write_cmd(struct pci_dev *pdev, u16 id, u32 data);
int proc_thermal_add(struct device *dev, struct proc_thermal_device *priv);
void proc_thermal_remove(struct proc_thermal_device *proc_priv);
int proc_thermal_suspend(struct device *dev);
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
index 01008ae00e7f..26bae0434829 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
@@ -24,19 +24,15 @@
static DEFINE_MUTEX(mbox_lock);
-static int send_mbox_cmd(struct pci_dev *pdev, u16 cmd_id, u32 cmd_data, u64 *cmd_resp)
+static int wait_for_mbox_ready(struct proc_thermal_device *proc_priv)
{
- struct proc_thermal_device *proc_priv;
u32 retries, data;
int ret;
- mutex_lock(&mbox_lock);
- proc_priv = pci_get_drvdata(pdev);
-
/* Poll for rb bit == 0 */
retries = MBOX_RETRY_COUNT;
do {
- data = readl((void __iomem *) (proc_priv->mmio_base + MBOX_OFFSET_INTERFACE));
+ data = readl(proc_priv->mmio_base + MBOX_OFFSET_INTERFACE);
if (data & BIT_ULL(MBOX_BUSY_BIT)) {
ret = -EBUSY;
continue;
@@ -45,53 +41,79 @@ static int send_mbox_cmd(struct pci_dev *pdev, u16 cmd_id, u32 cmd_data, u64 *cm
break;
} while (--retries);
+ return ret;
+}
+
+static int send_mbox_write_cmd(struct pci_dev *pdev, u16 id, u32 data)
+{
+ struct proc_thermal_device *proc_priv;
+ u32 reg_data;
+ int ret;
+
+ proc_priv = pci_get_drvdata(pdev);
+
+ mutex_lock(&mbox_lock);
+
+ ret = wait_for_mbox_ready(proc_priv);
if (ret)
goto unlock_mbox;
- if (cmd_id == MBOX_CMD_WORKLOAD_TYPE_WRITE)
- writel(cmd_data, (void __iomem *) ((proc_priv->mmio_base + MBOX_OFFSET_DATA)));
-
+ writel(data, (proc_priv->mmio_base + MBOX_OFFSET_DATA));
/* Write command register */
- data = BIT_ULL(MBOX_BUSY_BIT) | cmd_id;
- writel(data, (void __iomem *) ((proc_priv->mmio_base + MBOX_OFFSET_INTERFACE)));
+ reg_data = BIT_ULL(MBOX_BUSY_BIT) | id;
+ writel(reg_data, (proc_priv->mmio_base + MBOX_OFFSET_INTERFACE));
- /* Poll for rb bit == 0 */
- retries = MBOX_RETRY_COUNT;
- do {
- data = readl((void __iomem *) (proc_priv->mmio_base + MBOX_OFFSET_INTERFACE));
- if (data & BIT_ULL(MBOX_BUSY_BIT)) {
- ret = -EBUSY;
- continue;
- }
+ ret = wait_for_mbox_ready(proc_priv);
- if (data) {
- ret = -ENXIO;
- goto unlock_mbox;
- }
+unlock_mbox:
+ mutex_unlock(&mbox_lock);
+ return ret;
+}
- ret = 0;
+static int send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u32 data, u64 *resp)
+{
+ struct proc_thermal_device *proc_priv;
+ u32 reg_data;
+ int ret;
- if (!cmd_resp)
- break;
+ proc_priv = pci_get_drvdata(pdev);
- if (cmd_id == MBOX_CMD_WORKLOAD_TYPE_READ)
- *cmd_resp = readl((void __iomem *) (proc_priv->mmio_base + MBOX_OFFSET_DATA));
- else
- *cmd_resp = readq((void __iomem *) (proc_priv->mmio_base + MBOX_OFFSET_DATA));
+ mutex_lock(&mbox_lock);
- break;
- } while (--retries);
+ ret = wait_for_mbox_ready(proc_priv);
+ if (ret)
+ goto unlock_mbox;
+
+ writel(data, (proc_priv->mmio_base + MBOX_OFFSET_DATA));
+ /* Write command register */
+ reg_data = BIT_ULL(MBOX_BUSY_BIT) | id;
+ writel(reg_data, (proc_priv->mmio_base + MBOX_OFFSET_INTERFACE));
+
+ ret = wait_for_mbox_ready(proc_priv);
+ if (ret)
+ goto unlock_mbox;
+
+ if (id == MBOX_CMD_WORKLOAD_TYPE_READ)
+ *resp = readl(proc_priv->mmio_base + MBOX_OFFSET_DATA);
+ else
+ *resp = readq(proc_priv->mmio_base + MBOX_OFFSET_DATA);
unlock_mbox:
mutex_unlock(&mbox_lock);
return ret;
}
-int processor_thermal_send_mbox_cmd(struct pci_dev *pdev, u16 cmd_id, u32 cmd_data, u64 *cmd_resp)
+int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u32 data, u64 *resp)
{
- return send_mbox_cmd(pdev, cmd_id, cmd_data, cmd_resp);
+ return send_mbox_read_cmd(pdev, id, data, resp);
}
-EXPORT_SYMBOL_GPL(processor_thermal_send_mbox_cmd);
+EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_read_cmd, INT340X_THERMAL);
+
+int processor_thermal_send_mbox_write_cmd(struct pci_dev *pdev, u16 id, u32 data)
+{
+ return send_mbox_write_cmd(pdev, id, data);
+}
+EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_write_cmd, INT340X_THERMAL);
/* List of workload types */
static const char * const workload_types[] = {
@@ -104,7 +126,6 @@ static const char * const workload_types[] = {
NULL
};
-
static ssize_t workload_available_types_show(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -146,7 +167,7 @@ static ssize_t workload_type_store(struct device *dev,
data |= ret;
- ret = send_mbox_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_WRITE, data, NULL);
+ ret = send_mbox_write_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_WRITE, data);
if (ret)
return false;
@@ -161,7 +182,7 @@ static ssize_t workload_type_show(struct device *dev,
u64 cmd_resp;
int ret;
- ret = send_mbox_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_READ, 0, &cmd_resp);
+ ret = send_mbox_read_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_READ, 0, &cmd_resp);
if (ret)
return false;
@@ -186,8 +207,6 @@ static const struct attribute_group workload_req_attribute_group = {
.name = "workload_request"
};
-
-
static bool workload_req_created;
int proc_thermal_mbox_add(struct pci_dev *pdev, struct proc_thermal_device *proc_priv)
@@ -196,7 +215,7 @@ int proc_thermal_mbox_add(struct pci_dev *pdev, struct proc_thermal_device *proc
int ret;
/* Check if there is a mailbox support, if fails return success */
- ret = send_mbox_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_READ, 0, &cmd_resp);
+ ret = send_mbox_read_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_READ, 0, &cmd_resp);
if (ret)
return 0;
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
index e693ec8234fb..bebc7aaf3433 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
@@ -9,6 +9,8 @@
#include <linux/pci.h>
#include "processor_thermal_device.h"
+MODULE_IMPORT_NS(INT340X_THERMAL);
+
struct mmio_reg {
int read_only;
u32 offset;
@@ -194,8 +196,7 @@ static ssize_t rfi_restriction_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
- u16 cmd_id = 0x0008;
- u64 cmd_resp;
+ u16 id = 0x0008;
u32 input;
int ret;
@@ -203,7 +204,7 @@ static ssize_t rfi_restriction_store(struct device *dev,
if (ret)
return ret;
- ret = processor_thermal_send_mbox_cmd(to_pci_dev(dev), cmd_id, input, &cmd_resp);
+ ret = processor_thermal_send_mbox_write_cmd(to_pci_dev(dev), id, input);
if (ret)
return ret;
@@ -214,30 +215,30 @@ static ssize_t rfi_restriction_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- u16 cmd_id = 0x0007;
- u64 cmd_resp;
+ u16 id = 0x0007;
+ u64 resp;
int ret;
- ret = processor_thermal_send_mbox_cmd(to_pci_dev(dev), cmd_id, 0, &cmd_resp);
+ ret = processor_thermal_send_mbox_read_cmd(to_pci_dev(dev), id, 0, &resp);
if (ret)
return ret;
- return sprintf(buf, "%llu\n", cmd_resp);
+ return sprintf(buf, "%llu\n", resp);
}
static ssize_t ddr_data_rate_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- u16 cmd_id = 0x0107;
- u64 cmd_resp;
+ u16 id = 0x0107;
+ u64 resp;
int ret;
- ret = processor_thermal_send_mbox_cmd(to_pci_dev(dev), cmd_id, 0, &cmd_resp);
+ ret = processor_thermal_send_mbox_read_cmd(to_pci_dev(dev), id, 0, &resp);
if (ret)
return ret;
- return sprintf(buf, "%llu\n", cmd_resp);
+ return sprintf(buf, "%llu\n", resp);
}
static DEVICE_ATTR_RW(rfi_restriction);
--
2.17.1
On Wed, Dec 22, 2021 at 5:53 PM Sumeet Pawnikar
<[email protected]> wrote:
>
> The existing mail mechanism only supports writing of workload types.
> But mailbox command for RFIM (cmd = 0x08) also requires write operation
> which was ignored. This results in failing to store RFI restriction.
> This requires enhancing mailbox writes for non workload commands also.
> So, remove the check for MBOX_CMD_WORKLOAD_TYPE_WRITE in mailbox write,
> with this other write commands also can be supoorted. But at the same
> time, we have to make sure that there is no impact on read commands,
> by not writing anything in mailbox data register.
> To properly implement, add two separate functions for mbox read and write
> command for processor thermal workload command type. This helps to
> differentiate the read and write workload command types while sending mbox
> command.
>
> Fixes: 5d6fbc96bd36 ("thermal/drivers/int340x: processor_thermal: Export additional attributes")
> Signed-off-by: Sumeet Pawnikar <[email protected]>
> Cc: [email protected] # 5.14+
This requires an ACK from Srinivas.
> ---
> .../processor_thermal_device.h | 3 +-
> .../int340x_thermal/processor_thermal_mbox.c | 101 +++++++++++-------
> .../int340x_thermal/processor_thermal_rfim.c | 23 ++--
> 3 files changed, 74 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> index be27f633e40a..43def8c5d2ce 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> @@ -80,7 +80,8 @@ void proc_thermal_rfim_remove(struct pci_dev *pdev);
> int proc_thermal_mbox_add(struct pci_dev *pdev, struct proc_thermal_device *proc_priv);
> void proc_thermal_mbox_remove(struct pci_dev *pdev);
>
> -int processor_thermal_send_mbox_cmd(struct pci_dev *pdev, u16 cmd_id, u32 cmd_data, u64 *cmd_resp);
> +int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u32 data, u64 *resp);
> +int processor_thermal_send_mbox_write_cmd(struct pci_dev *pdev, u16 id, u32 data);
> int proc_thermal_add(struct device *dev, struct proc_thermal_device *priv);
> void proc_thermal_remove(struct proc_thermal_device *proc_priv);
> int proc_thermal_suspend(struct device *dev);
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
> index 01008ae00e7f..26bae0434829 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
> @@ -24,19 +24,15 @@
>
> static DEFINE_MUTEX(mbox_lock);
>
> -static int send_mbox_cmd(struct pci_dev *pdev, u16 cmd_id, u32 cmd_data, u64 *cmd_resp)
> +static int wait_for_mbox_ready(struct proc_thermal_device *proc_priv)
> {
> - struct proc_thermal_device *proc_priv;
> u32 retries, data;
> int ret;
>
> - mutex_lock(&mbox_lock);
> - proc_priv = pci_get_drvdata(pdev);
> -
> /* Poll for rb bit == 0 */
> retries = MBOX_RETRY_COUNT;
> do {
> - data = readl((void __iomem *) (proc_priv->mmio_base + MBOX_OFFSET_INTERFACE));
> + data = readl(proc_priv->mmio_base + MBOX_OFFSET_INTERFACE);
> if (data & BIT_ULL(MBOX_BUSY_BIT)) {
> ret = -EBUSY;
> continue;
> @@ -45,53 +41,79 @@ static int send_mbox_cmd(struct pci_dev *pdev, u16 cmd_id, u32 cmd_data, u64 *cm
> break;
> } while (--retries);
>
> + return ret;
> +}
> +
> +static int send_mbox_write_cmd(struct pci_dev *pdev, u16 id, u32 data)
> +{
> + struct proc_thermal_device *proc_priv;
> + u32 reg_data;
> + int ret;
> +
> + proc_priv = pci_get_drvdata(pdev);
> +
> + mutex_lock(&mbox_lock);
> +
> + ret = wait_for_mbox_ready(proc_priv);
> if (ret)
> goto unlock_mbox;
>
> - if (cmd_id == MBOX_CMD_WORKLOAD_TYPE_WRITE)
> - writel(cmd_data, (void __iomem *) ((proc_priv->mmio_base + MBOX_OFFSET_DATA)));
> -
> + writel(data, (proc_priv->mmio_base + MBOX_OFFSET_DATA));
> /* Write command register */
> - data = BIT_ULL(MBOX_BUSY_BIT) | cmd_id;
> - writel(data, (void __iomem *) ((proc_priv->mmio_base + MBOX_OFFSET_INTERFACE)));
> + reg_data = BIT_ULL(MBOX_BUSY_BIT) | id;
> + writel(reg_data, (proc_priv->mmio_base + MBOX_OFFSET_INTERFACE));
>
> - /* Poll for rb bit == 0 */
> - retries = MBOX_RETRY_COUNT;
> - do {
> - data = readl((void __iomem *) (proc_priv->mmio_base + MBOX_OFFSET_INTERFACE));
> - if (data & BIT_ULL(MBOX_BUSY_BIT)) {
> - ret = -EBUSY;
> - continue;
> - }
> + ret = wait_for_mbox_ready(proc_priv);
>
> - if (data) {
> - ret = -ENXIO;
> - goto unlock_mbox;
> - }
> +unlock_mbox:
> + mutex_unlock(&mbox_lock);
> + return ret;
> +}
>
> - ret = 0;
> +static int send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u32 data, u64 *resp)
> +{
> + struct proc_thermal_device *proc_priv;
> + u32 reg_data;
> + int ret;
>
> - if (!cmd_resp)
> - break;
> + proc_priv = pci_get_drvdata(pdev);
>
> - if (cmd_id == MBOX_CMD_WORKLOAD_TYPE_READ)
> - *cmd_resp = readl((void __iomem *) (proc_priv->mmio_base + MBOX_OFFSET_DATA));
> - else
> - *cmd_resp = readq((void __iomem *) (proc_priv->mmio_base + MBOX_OFFSET_DATA));
> + mutex_lock(&mbox_lock);
>
> - break;
> - } while (--retries);
> + ret = wait_for_mbox_ready(proc_priv);
> + if (ret)
> + goto unlock_mbox;
> +
> + writel(data, (proc_priv->mmio_base + MBOX_OFFSET_DATA));
> + /* Write command register */
> + reg_data = BIT_ULL(MBOX_BUSY_BIT) | id;
> + writel(reg_data, (proc_priv->mmio_base + MBOX_OFFSET_INTERFACE));
> +
> + ret = wait_for_mbox_ready(proc_priv);
> + if (ret)
> + goto unlock_mbox;
> +
> + if (id == MBOX_CMD_WORKLOAD_TYPE_READ)
> + *resp = readl(proc_priv->mmio_base + MBOX_OFFSET_DATA);
> + else
> + *resp = readq(proc_priv->mmio_base + MBOX_OFFSET_DATA);
>
> unlock_mbox:
> mutex_unlock(&mbox_lock);
> return ret;
> }
>
> -int processor_thermal_send_mbox_cmd(struct pci_dev *pdev, u16 cmd_id, u32 cmd_data, u64 *cmd_resp)
> +int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u32 data, u64 *resp)
> {
> - return send_mbox_cmd(pdev, cmd_id, cmd_data, cmd_resp);
> + return send_mbox_read_cmd(pdev, id, data, resp);
> }
> -EXPORT_SYMBOL_GPL(processor_thermal_send_mbox_cmd);
> +EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_read_cmd, INT340X_THERMAL);
> +
> +int processor_thermal_send_mbox_write_cmd(struct pci_dev *pdev, u16 id, u32 data)
> +{
> + return send_mbox_write_cmd(pdev, id, data);
> +}
> +EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_write_cmd, INT340X_THERMAL);
>
> /* List of workload types */
> static const char * const workload_types[] = {
> @@ -104,7 +126,6 @@ static const char * const workload_types[] = {
> NULL
> };
>
> -
> static ssize_t workload_available_types_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -146,7 +167,7 @@ static ssize_t workload_type_store(struct device *dev,
>
> data |= ret;
>
> - ret = send_mbox_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_WRITE, data, NULL);
> + ret = send_mbox_write_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_WRITE, data);
> if (ret)
> return false;
>
> @@ -161,7 +182,7 @@ static ssize_t workload_type_show(struct device *dev,
> u64 cmd_resp;
> int ret;
>
> - ret = send_mbox_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_READ, 0, &cmd_resp);
> + ret = send_mbox_read_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_READ, 0, &cmd_resp);
> if (ret)
> return false;
>
> @@ -186,8 +207,6 @@ static const struct attribute_group workload_req_attribute_group = {
> .name = "workload_request"
> };
>
> -
> -
> static bool workload_req_created;
>
> int proc_thermal_mbox_add(struct pci_dev *pdev, struct proc_thermal_device *proc_priv)
> @@ -196,7 +215,7 @@ int proc_thermal_mbox_add(struct pci_dev *pdev, struct proc_thermal_device *proc
> int ret;
>
> /* Check if there is a mailbox support, if fails return success */
> - ret = send_mbox_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_READ, 0, &cmd_resp);
> + ret = send_mbox_read_cmd(pdev, MBOX_CMD_WORKLOAD_TYPE_READ, 0, &cmd_resp);
> if (ret)
> return 0;
>
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> index e693ec8234fb..bebc7aaf3433 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> @@ -9,6 +9,8 @@
> #include <linux/pci.h>
> #include "processor_thermal_device.h"
>
> +MODULE_IMPORT_NS(INT340X_THERMAL);
> +
> struct mmio_reg {
> int read_only;
> u32 offset;
> @@ -194,8 +196,7 @@ static ssize_t rfi_restriction_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - u16 cmd_id = 0x0008;
> - u64 cmd_resp;
> + u16 id = 0x0008;
> u32 input;
> int ret;
>
> @@ -203,7 +204,7 @@ static ssize_t rfi_restriction_store(struct device *dev,
> if (ret)
> return ret;
>
> - ret = processor_thermal_send_mbox_cmd(to_pci_dev(dev), cmd_id, input, &cmd_resp);
> + ret = processor_thermal_send_mbox_write_cmd(to_pci_dev(dev), id, input);
> if (ret)
> return ret;
>
> @@ -214,30 +215,30 @@ static ssize_t rfi_restriction_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - u16 cmd_id = 0x0007;
> - u64 cmd_resp;
> + u16 id = 0x0007;
> + u64 resp;
> int ret;
>
> - ret = processor_thermal_send_mbox_cmd(to_pci_dev(dev), cmd_id, 0, &cmd_resp);
> + ret = processor_thermal_send_mbox_read_cmd(to_pci_dev(dev), id, 0, &resp);
> if (ret)
> return ret;
>
> - return sprintf(buf, "%llu\n", cmd_resp);
> + return sprintf(buf, "%llu\n", resp);
> }
>
> static ssize_t ddr_data_rate_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - u16 cmd_id = 0x0107;
> - u64 cmd_resp;
> + u16 id = 0x0107;
> + u64 resp;
> int ret;
>
> - ret = processor_thermal_send_mbox_cmd(to_pci_dev(dev), cmd_id, 0, &cmd_resp);
> + ret = processor_thermal_send_mbox_read_cmd(to_pci_dev(dev), id, 0, &resp);
> if (ret)
> return ret;
>
> - return sprintf(buf, "%llu\n", cmd_resp);
> + return sprintf(buf, "%llu\n", resp);
> }
>
> static DEVICE_ATTR_RW(rfi_restriction);
> --
> 2.17.1
>
On Wed, 2021-12-22 at 17:56 +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 22, 2021 at 5:53 PM Sumeet Pawnikar
> <[email protected]> wrote:
> >
> > The existing mail mechanism only supports writing of workload
> > types.
> > But mailbox command for RFIM (cmd = 0x08) also requires write
> > operation
> > which was ignored. This results in failing to store RFI
> > restriction.
> > This requires enhancing mailbox writes for non workload commands
> > also.
> > So, remove the check for MBOX_CMD_WORKLOAD_TYPE_WRITE in mailbox
> > write,
> > with this other write commands also can be supoorted. But at the
> > same
> > time, we have to make sure that there is no impact on read
> > commands,
> > by not writing anything in mailbox data register.
> > To properly implement, add two separate functions for mbox read and
> > write
> > command for processor thermal workload command type. This helps to
> > differentiate the read and write workload command types while
> > sending mbox
> > command.
> >
> > Fixes: 5d6fbc96bd36 ("thermal/drivers/int340x: processor_thermal:
> > Export additional attributes")
> > Signed-off-by: Sumeet Pawnikar <[email protected]>
> > Cc: [email protected] # 5.14+
>
> This requires an ACK from Srinivas.
I found some more issues in this patch after my prior internal review.
1. Subject of patch should be what is this patch for rather than how.
Something like:
Fix RFIM mailbox write commands
2. There is one issue in the code below.
>
> > ---
> >
[...]
> > +static int send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u32
> > data, u64 *resp)
There is no use of "data" argument for read after spilt of read and
write commands, if you look at the code before.
> > +{
> > + struct proc_thermal_device *proc_priv;
> > + u32 reg_data;
> > + int ret;
> >
> > - if (!cmd_resp)
> > - break;
> > + proc_priv = pci_get_drvdata(pdev);
> >
> > - if (cmd_id == MBOX_CMD_WORKLOAD_TYPE_READ)
> > - *cmd_resp = readl((void __iomem *)
> > (proc_priv->mmio_base + MBOX_OFFSET_DATA));
> > - else
> > - *cmd_resp = readq((void __iomem *)
> > (proc_priv->mmio_base + MBOX_OFFSET_DATA));
> > + mutex_lock(&mbox_lock);
> >
> > - break;
> > - } while (--retries);
> > + ret = wait_for_mbox_ready(proc_priv);
> > + if (ret)
> > + goto unlock_mbox;
> > +
> > + writel(data, (proc_priv->mmio_base + MBOX_OFFSET_DATA));
The above is not required for read. This is what we wanted to avoid for
reads.
Thanks,
Srinivas
> > + /* Write command register */
> > + reg_data = BIT_ULL(MBOX_BUSY_BIT) | id;
> > + writel(reg_data, (proc_priv->mmio_base +
> > MBOX_OFFSET_INTERFACE));
> > +
> > + ret = wait_for_mbox_ready(proc_priv);
> > + if (ret)
> > + goto unlock_mbox;
> > +
> > + if (id == MBOX_CMD_WORKLOAD_TYPE_READ)
> > + *resp = readl(proc_priv->mmio_base +
> > MBOX_OFFSET_DATA);
> > + else
> > + *resp = readq(proc_priv->mmio_base +
> > MBOX_OFFSET_DATA);
> >
> > unlock_mbox:
> > mutex_unlock(&mbox_lock);
> > return ret;
> > }
> >
> > -int processor_thermal_send_mbox_cmd(struct pci_dev *pdev, u16
> > cmd_id, u32 cmd_data, u64 *cmd_resp)
> > +int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev, u16
> > id, u32 data, u64 *resp)
> > {
> > - return send_mbox_cmd(pdev, cmd_id, cmd_data, cmd_resp);
> > + return send_mbox_read_cmd(pdev, id, data, resp);
> > }
> > -EXPORT_SYMBOL_GPL(processor_thermal_send_mbox_cmd);
> > +EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_read_cmd,
> > INT340X_THERMAL);
> > +
> > +int processor_thermal_send_mbox_write_cmd(struct pci_dev *pdev,
> > u16 id, u32 data)
> > +{
> > + return send_mbox_write_cmd(pdev, id, data);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_write_cmd,
> > INT340X_THERMAL);
> >
> > /* List of workload types */
> > static const char * const workload_types[] = {
> > @@ -104,7 +126,6 @@ static const char * const workload_types[] = {
> > NULL
> > };
> >
> -----Original Message-----
> From: srinivas pandruvada <[email protected]>
> Sent: Wednesday, December 22, 2021 11:16 PM
> To: Rafael J. Wysocki <[email protected]>; Pawnikar, Sumeet R
> <[email protected]>
> Cc: Daniel Lezcano <[email protected]>; Linux PM <linux-
> [email protected]>; Linux Kernel Mailing List <linux-
> [email protected]>; Stable <[email protected]>
> Subject: Re: [PATCH] thermal/drivers/int340x: add functions for mbox read
> and write commands
>
> On Wed, 2021-12-22 at 17:56 +0100, Rafael J. Wysocki wrote:
> > On Wed, Dec 22, 2021 at 5:53 PM Sumeet Pawnikar
> > <[email protected]> wrote:
> > >
> > > The existing mail mechanism only supports writing of workload types.
> > > But mailbox command for RFIM (cmd = 0x08) also requires write
> > > operation which was ignored. This results in failing to store RFI
> > > restriction.
> > > This requires enhancing mailbox writes for non workload commands
> > > also.
> > > So, remove the check for MBOX_CMD_WORKLOAD_TYPE_WRITE in
> mailbox
> > > write, with this other write commands also can be supoorted. But at
> > > the same time, we have to make sure that there is no impact on read
> > > commands, by not writing anything in mailbox data register.
> > > To properly implement, add two separate functions for mbox read and
> > > write command for processor thermal workload command type. This
> > > helps to differentiate the read and write workload command types
> > > while sending mbox command.
> > >
> > > Fixes: 5d6fbc96bd36 ("thermal/drivers/int340x: processor_thermal:
> > > Export additional attributes")
> > > Signed-off-by: Sumeet Pawnikar <[email protected]>
> > > Cc: [email protected] # 5.14+
> >
> > This requires an ACK from Srinivas.
>
> I found some more issues in this patch after my prior internal review.
>
> 1. Subject of patch should be what is this patch for rather than how.
> Something like:
> Fix RFIM mailbox write commands
>
Sure, let me follow this.
> 2. There is one issue in the code below.
>
> >
> > > ---
> > >
>
> [...]
>
> > > +static int send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u32
> > > data, u64 *resp)
> There is no use of "data" argument for read after spilt of read and write
> commands, if you look at the code before.
>
Yes, I will update according to this.
> > > +{
> > > + struct proc_thermal_device *proc_priv;
> > > + u32 reg_data;
> > > + int ret;
> > >
> > > - if (!cmd_resp)
> > > - break;
> > > + proc_priv = pci_get_drvdata(pdev);
> > >
> > > - if (cmd_id == MBOX_CMD_WORKLOAD_TYPE_READ)
> > > - *cmd_resp = readl((void __iomem *)
> > > (proc_priv->mmio_base + MBOX_OFFSET_DATA));
> > > - else
> > > - *cmd_resp = readq((void __iomem *)
> > > (proc_priv->mmio_base + MBOX_OFFSET_DATA));
> > > + mutex_lock(&mbox_lock);
> > >
> > > - break;
> > > - } while (--retries);
> > > + ret = wait_for_mbox_ready(proc_priv);
> > > + if (ret)
> > > + goto unlock_mbox;
> > > +
> > > + writel(data, (proc_priv->mmio_base + MBOX_OFFSET_DATA));
> The above is not required for read. This is what we wanted to avoid for
> reads.
>
Let me remove this and test new changes and post V2 for review.
Thanks,
Sumeet.
> Thanks,
> Srinivas
>
> > > + /* Write command register */
> > > + reg_data = BIT_ULL(MBOX_BUSY_BIT) | id;
> > > + writel(reg_data, (proc_priv->mmio_base +
> > > MBOX_OFFSET_INTERFACE));
> > > +
> > > + ret = wait_for_mbox_ready(proc_priv);
> > > + if (ret)
> > > + goto unlock_mbox;
> > > +
> > > + if (id == MBOX_CMD_WORKLOAD_TYPE_READ)
> > > + *resp = readl(proc_priv->mmio_base +
> > > MBOX_OFFSET_DATA);
> > > + else
> > > + *resp = readq(proc_priv->mmio_base +
> > > MBOX_OFFSET_DATA);
> > >
> > > unlock_mbox:
> > > mutex_unlock(&mbox_lock);
> > > return ret;
> > > }
> > >
> > > -int processor_thermal_send_mbox_cmd(struct pci_dev *pdev, u16
> > > cmd_id, u32 cmd_data, u64 *cmd_resp)
> > > +int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev,
> u16
> > > id, u32 data, u64 *resp)
> > > {
> > > - return send_mbox_cmd(pdev, cmd_id, cmd_data, cmd_resp);
> > > + return send_mbox_read_cmd(pdev, id, data, resp);
> > > }
> > > -EXPORT_SYMBOL_GPL(processor_thermal_send_mbox_cmd);
> > > +EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_read_cmd,
> > > INT340X_THERMAL);
> > > +
> > > +int processor_thermal_send_mbox_write_cmd(struct pci_dev *pdev,
> > > u16 id, u32 data)
> > > +{
> > > + return send_mbox_write_cmd(pdev, id, data); }
> > > +EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_write_cmd,
> > > INT340X_THERMAL);
> > >
> > > /* List of workload types */
> > > static const char * const workload_types[] = { @@ -104,7 +126,6 @@
> > > static const char * const workload_types[] = {
> > > NULL
> > > };
> > >
>