| Message ID | 20230620230150.3068704-3-srinivas.pandruvada@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | thermal: processor_thermal: Suport workload hint | expand |
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"); >
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) +{ + 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");
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(-)