Re: [PATCH 2/7] thermal: int340x: processor_thermal: Add interrupt configuration

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

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");
>