[2/7] thermal: int340x: processor_thermal: Add interrupt configuration
Commit Message
Some features on this PCI devices require interrupt support. Here
interrupts are enabled/disabled via sending mailbox commands. The
mailbox command ID is 0x1E for read and 0x1F for write.
The interrupt configuration will require mutex protection as it
involved read-modify-write operation. Since mutex are already used
in the mailbox read/write functions: send_mbox_write_cmd() and
send_mbox_read_cmd(), there will be double locking. But, this can
be avoided by moving mutexes from mailbox read/write processing
functions to the calling (exported) functions.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
.../processor_thermal_device.h | 2 +
.../int340x_thermal/processor_thermal_mbox.c | 85 ++++++++++++++-----
2 files changed, 68 insertions(+), 19 deletions(-)
Comments
On Tue, 2023-06-20 at 16:01 -0700, Srinivas Pandruvada wrote:
> Some features on this PCI devices require interrupt support. Here
> interrupts are enabled/disabled via sending mailbox commands. The
> mailbox command ID is 0x1E for read and 0x1F for write.
>
> The interrupt configuration will require mutex protection as it
> involved read-modify-write operation. Since mutex are already used
> in the mailbox read/write functions: send_mbox_write_cmd() and
> send_mbox_read_cmd(), there will be double locking. But, this can
> be avoided by moving mutexes from mailbox read/write processing
> functions to the calling (exported) functions.
>
> Signed-off-by: Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>
> ---
> .../processor_thermal_device.h | 2 +
> .../int340x_thermal/processor_thermal_mbox.c | 85 ++++++++++++++---
> --
> 2 files changed, 68 insertions(+), 19 deletions(-)
>
> diff --git
> a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> index 7cdeca2edc21..defc919cb020 100644
> ---
> a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> +++
> b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> @@ -91,6 +91,8 @@ void proc_thermal_wlt_req_remove(struct pci_dev
> *pdev);
>
> int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev, u16
> id, u64 *resp);
> int processor_thermal_send_mbox_write_cmd(struct pci_dev *pdev, u16
> id, u32 data);
> +int processor_thermal_mbox_interrupt_config(struct pci_dev *pdev,
> bool enable, int enable_bit,
> + int time_window);
> 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 ec766c5615b7..7ef0af3f5bef 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
> @@ -45,23 +45,16 @@ static int send_mbox_write_cmd(struct pci_dev
> *pdev, u16 id, u32 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;
> + return ret;
>
> 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);
> -
> -unlock_mbox:
> - mutex_unlock(&mbox_lock);
> - return ret;
> + return wait_for_mbox_ready(proc_priv);
> }
>
> static int send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u64
> *resp)
> @@ -71,12 +64,9 @@ static int send_mbox_read_cmd(struct pci_dev
> *pdev, u16 id, u64 *resp)
> int ret;
>
> proc_priv = pci_get_drvdata(pdev);
> -
> - mutex_lock(&mbox_lock);
> -
> ret = wait_for_mbox_ready(proc_priv);
> if (ret)
> - goto unlock_mbox;
> + return ret;
>
> /* Write command register */
> reg_data = BIT_ULL(MBOX_BUSY_BIT) | id;
> @@ -84,28 +74,85 @@ static int send_mbox_read_cmd(struct pci_dev
> *pdev, u16 id, u64 *resp)
>
> ret = wait_for_mbox_ready(proc_priv);
> if (ret)
> - goto unlock_mbox;
> + return ret;
>
> 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;
> + return 0;
> }
>
> int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev, u16
> id, u64 *resp)
> {
> - return send_mbox_read_cmd(pdev, id, resp);
> + int ret;
> +
> + mutex_lock(&mbox_lock);
> + ret = send_mbox_read_cmd(pdev, id, resp);
> + mutex_unlock(&mbox_lock);
> +
> + return ret;
> }
> 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);
> + int ret;
> +
> + mutex_lock(&mbox_lock);
> + ret = send_mbox_write_cmd(pdev, id, data);
> + mutex_unlock(&mbox_lock);
> +
> + return ret;
> }
> EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_write_cmd,
> INT340X_THERMAL);
>
> +#define MBOX_CAMARILLO_RD_INTR_CONFIG 0x1E
> +#define MBOX_CAMARILLO_WR_INTR_CONFIG 0x1F
> +#define WLT_TW_MASK GENMASK_ULL(30, 24)
> +#define SOC_PREDICTION_TW_SHIFT 24
> +
> +int processor_thermal_mbox_interrupt_config(struct pci_dev *pdev,
> bool enable,
> + int enable_bit, int
> time_window)
All the callers of this API in this patch series uses
SOC_WLT_PREDICTION_INT_ENABLE_BIT as the enable_bit, so this parameter
is redundant?
or do we expect a different enable_bit on other/future platforms?
thanks,
rui
> +{
> + u64 data;
> + int ret;
> +
> + if (!pdev)
> + return -ENODEV;
> +
> + mutex_lock(&mbox_lock);
> +
> + /* Do read modify write for MBOX_CAMARILLO_RD_INTR_CONFIG */
> +
> + ret = send_mbox_read_cmd(pdev,
> MBOX_CAMARILLO_RD_INTR_CONFIG, &data);
> + if (ret) {
> + dev_err(&pdev->dev, "MBOX_CAMARILLO_RD_INTR_CONFIG
> failed\n");
> + goto unlock;
> + }
> +
> + if (time_window >= 0) {
> + data &= ~WLT_TW_MASK;
> +
> + /* Program notification delay */
> + data |= (time_window << SOC_PREDICTION_TW_SHIFT);
> + }
> +
> + if (enable)
> + data |= BIT(enable_bit);
> + else
> + data &= ~BIT(enable_bit);
> +
> + ret = send_mbox_write_cmd(pdev,
> MBOX_CAMARILLO_WR_INTR_CONFIG, data);
> + if (ret)
> + dev_err(&pdev->dev, "MBOX_CAMARILLO_WR_INTR_CONFIG
> failed\n");
> +
> +unlock:
> + mutex_unlock(&mbox_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(processor_thermal_mbox_interrupt_config,
> INT340X_THERMAL);
> +
> MODULE_LICENSE("GPL v2");
On Wed, 2023-06-21 at 14:50 +0000, Zhang, Rui wrote:
> On Tue, 2023-06-20 at 16:01 -0700, Srinivas Pandruvada wrote:
> > Some features on this PCI devices require interrupt support. Here
> > interrupts are enabled/disabled via sending mailbox commands. The
> > mailbox command ID is 0x1E for read and 0x1F for write.
> >
> > The interrupt configuration will require mutex protection as it
> > involved read-modify-write operation. Since mutex are already used
> > in the mailbox read/write functions: send_mbox_write_cmd() and
> > send_mbox_read_cmd(), there will be double locking. But, this can
> > be avoided by moving mutexes from mailbox read/write processing
> > functions to the calling (exported) functions.
> >
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>
> > ---
> > .../processor_thermal_device.h | 2 +
> > .../int340x_thermal/processor_thermal_mbox.c | 85 ++++++++++++++-
> > --
> > --
> > 2 files changed, 68 insertions(+), 19 deletions(-)
> >
> > diff --git
> > a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> > b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> > index 7cdeca2edc21..defc919cb020 100644
> > ---
> > a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> > +++
> > b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> > @@ -91,6 +91,8 @@ void proc_thermal_wlt_req_remove(struct pci_dev
> > *pdev);
> >
> > int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev, u16
> > id, u64 *resp);
> > int processor_thermal_send_mbox_write_cmd(struct pci_dev *pdev,
> > u16
> > id, u32 data);
> > +int processor_thermal_mbox_interrupt_config(struct pci_dev *pdev,
> > bool enable, int enable_bit,
> > + int time_window);
> > 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 ec766c5615b7..7ef0af3f5bef 100644
> > ---
> > a/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
> > +++
> > b/drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.c
> > @@ -45,23 +45,16 @@ static int send_mbox_write_cmd(struct pci_dev
> > *pdev, u16 id, u32 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;
> > + return ret;
> >
> > 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);
> > -
> > -unlock_mbox:
> > - mutex_unlock(&mbox_lock);
> > - return ret;
> > + return wait_for_mbox_ready(proc_priv);
> > }
> >
> > static int send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u64
> > *resp)
> > @@ -71,12 +64,9 @@ static int send_mbox_read_cmd(struct pci_dev
> > *pdev, u16 id, u64 *resp)
> > int ret;
> >
> > proc_priv = pci_get_drvdata(pdev);
> > -
> > - mutex_lock(&mbox_lock);
> > -
> > ret = wait_for_mbox_ready(proc_priv);
> > if (ret)
> > - goto unlock_mbox;
> > + return ret;
> >
> > /* Write command register */
> > reg_data = BIT_ULL(MBOX_BUSY_BIT) | id;
> > @@ -84,28 +74,85 @@ static int send_mbox_read_cmd(struct pci_dev
> > *pdev, u16 id, u64 *resp)
> >
> > ret = wait_for_mbox_ready(proc_priv);
> > if (ret)
> > - goto unlock_mbox;
> > + return ret;
> >
> > 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;
> > + return 0;
> > }
> >
> > int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev, u16
> > id, u64 *resp)
> > {
> > - return send_mbox_read_cmd(pdev, id, resp);
> > + int ret;
> > +
> > + mutex_lock(&mbox_lock);
> > + ret = send_mbox_read_cmd(pdev, id, resp);
> > + mutex_unlock(&mbox_lock);
> > +
> > + return ret;
> > }
> > 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);
> > + int ret;
> > +
> > + mutex_lock(&mbox_lock);
> > + ret = send_mbox_write_cmd(pdev, id, data);
> > + mutex_unlock(&mbox_lock);
> > +
> > + return ret;
> > }
> > EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_write_cmd,
> > INT340X_THERMAL);
> >
> > +#define MBOX_CAMARILLO_RD_INTR_CONFIG 0x1E
> > +#define MBOX_CAMARILLO_WR_INTR_CONFIG 0x1F
> > +#define WLT_TW_MASK GENMASK_ULL(30, 24)
> > +#define SOC_PREDICTION_TW_SHIFT 24
> > +
> > +int processor_thermal_mbox_interrupt_config(struct pci_dev *pdev,
> > bool enable,
> > + int enable_bit, int
> > time_window)
>
> All the callers of this API in this patch series uses
> SOC_WLT_PREDICTION_INT_ENABLE_BIT as the enable_bit, so this
> parameter
> is redundant?
> or do we expect a different enable_bit on other/future platforms?
>
I will submit another patch for enabling interrupt for "power floor",
that is another bit.
Thanks,
Srinivas
> thanks,
> rui
>
> > +{
> > + u64 data;
> > + int ret;
> > +
> > + if (!pdev)
> > + return -ENODEV;
> > +
> > + mutex_lock(&mbox_lock);
> > +
> > + /* Do read modify write for MBOX_CAMARILLO_RD_INTR_CONFIG
> > */
> > +
> > + ret = send_mbox_read_cmd(pdev,
> > MBOX_CAMARILLO_RD_INTR_CONFIG, &data);
> > + if (ret) {
> > + dev_err(&pdev->dev, "MBOX_CAMARILLO_RD_INTR_CONFIG
> > failed\n");
> > + goto unlock;
> > + }
> > +
> > + if (time_window >= 0) {
> > + data &= ~WLT_TW_MASK;
> > +
> > + /* Program notification delay */
> > + data |= (time_window << SOC_PREDICTION_TW_SHIFT);
> > + }
> > +
> > + if (enable)
> > + data |= BIT(enable_bit);
> > + else
> > + data &= ~BIT(enable_bit);
> > +
> > + ret = send_mbox_write_cmd(pdev,
> > MBOX_CAMARILLO_WR_INTR_CONFIG, data);
> > + if (ret)
> > + dev_err(&pdev->dev, "MBOX_CAMARILLO_WR_INTR_CONFIG
> > failed\n");
> > +
> > +unlock:
> > + mutex_unlock(&mbox_lock);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(processor_thermal_mbox_interrupt_config,
> > INT340X_THERMAL);
> > +
> > MODULE_LICENSE("GPL v2");
>
@@ -91,6 +91,8 @@ void proc_thermal_wlt_req_remove(struct pci_dev *pdev);
int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u64 *resp);
int processor_thermal_send_mbox_write_cmd(struct pci_dev *pdev, u16 id, u32 data);
+int processor_thermal_mbox_interrupt_config(struct pci_dev *pdev, bool enable, int enable_bit,
+ int time_window);
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);
@@ -45,23 +45,16 @@ static int send_mbox_write_cmd(struct pci_dev *pdev, u16 id, u32 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;
+ return ret;
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);
-
-unlock_mbox:
- mutex_unlock(&mbox_lock);
- return ret;
+ return wait_for_mbox_ready(proc_priv);
}
static int send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u64 *resp)
@@ -71,12 +64,9 @@ static int send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u64 *resp)
int ret;
proc_priv = pci_get_drvdata(pdev);
-
- mutex_lock(&mbox_lock);
-
ret = wait_for_mbox_ready(proc_priv);
if (ret)
- goto unlock_mbox;
+ return ret;
/* Write command register */
reg_data = BIT_ULL(MBOX_BUSY_BIT) | id;
@@ -84,28 +74,85 @@ static int send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u64 *resp)
ret = wait_for_mbox_ready(proc_priv);
if (ret)
- goto unlock_mbox;
+ return ret;
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;
+ return 0;
}
int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u64 *resp)
{
- return send_mbox_read_cmd(pdev, id, resp);
+ int ret;
+
+ mutex_lock(&mbox_lock);
+ ret = send_mbox_read_cmd(pdev, id, resp);
+ mutex_unlock(&mbox_lock);
+
+ return ret;
}
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);
+ int ret;
+
+ mutex_lock(&mbox_lock);
+ ret = send_mbox_write_cmd(pdev, id, data);
+ mutex_unlock(&mbox_lock);
+
+ return ret;
}
EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_write_cmd, INT340X_THERMAL);
+#define MBOX_CAMARILLO_RD_INTR_CONFIG 0x1E
+#define MBOX_CAMARILLO_WR_INTR_CONFIG 0x1F
+#define WLT_TW_MASK GENMASK_ULL(30, 24)
+#define SOC_PREDICTION_TW_SHIFT 24
+
+int processor_thermal_mbox_interrupt_config(struct pci_dev *pdev, bool enable,
+ int enable_bit, int time_window)
+{
+ u64 data;
+ int ret;
+
+ if (!pdev)
+ return -ENODEV;
+
+ mutex_lock(&mbox_lock);
+
+ /* Do read modify write for MBOX_CAMARILLO_RD_INTR_CONFIG */
+
+ ret = send_mbox_read_cmd(pdev, MBOX_CAMARILLO_RD_INTR_CONFIG, &data);
+ if (ret) {
+ dev_err(&pdev->dev, "MBOX_CAMARILLO_RD_INTR_CONFIG failed\n");
+ goto unlock;
+ }
+
+ if (time_window >= 0) {
+ data &= ~WLT_TW_MASK;
+
+ /* Program notification delay */
+ data |= (time_window << SOC_PREDICTION_TW_SHIFT);
+ }
+
+ if (enable)
+ data |= BIT(enable_bit);
+ else
+ data &= ~BIT(enable_bit);
+
+ ret = send_mbox_write_cmd(pdev, MBOX_CAMARILLO_WR_INTR_CONFIG, data);
+ if (ret)
+ dev_err(&pdev->dev, "MBOX_CAMARILLO_WR_INTR_CONFIG failed\n");
+
+unlock:
+ mutex_unlock(&mbox_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_NS_GPL(processor_thermal_mbox_interrupt_config, INT340X_THERMAL);
+
MODULE_LICENSE("GPL v2");