Re: [PATCH v2 06/36] platform/x86: intel_scu_ipc: Fix interrupt support

[Andy Shevchenko]

On Wed, Jan 08, 2020 at 02:41:31PM +0300, Mika Westerberg wrote:
> Currently the driver has disabled interrupt support for Tangier but
> actually interrupt works just fine if the command is not written twice
> in a row. Also we need to ack the interrupt in the handler.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
>  drivers/platform/x86/intel_scu_ipc.c | 48 ++++++----------------------
>  1 file changed, 10 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> index b39680b53387..43eaf9400c67 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -58,24 +58,6 @@
>  #define IPC_RWBUF_SIZE    20		/* IPC Read buffer Size */
>  #define IPC_IOC	          0x100		/* IPC command register IOC bit */
>  
> -#define PCI_DEVICE_ID_PENWELL		0x080e
> -#define PCI_DEVICE_ID_CLOVERVIEW	0x08ea
> -#define PCI_DEVICE_ID_TANGIER		0x11a0
> -
> -/* intel scu ipc driver data */
> -struct intel_scu_ipc_pdata_t {
> -	u8 irq_mode;
> -};
> -
> -/* Penwell and Cloverview */
> -static const struct intel_scu_ipc_pdata_t intel_scu_ipc_penwell_pdata = {
> -	.irq_mode = 1,
> -};
> -
> -static const struct intel_scu_ipc_pdata_t intel_scu_ipc_tangier_pdata = {
> -	.irq_mode = 0,
> -};
> -
>  struct intel_scu_ipc_dev {
>  	struct device *dev;
>  	void __iomem *ipc_base;
> @@ -86,6 +68,7 @@ struct intel_scu_ipc_dev {
>  static struct intel_scu_ipc_dev  ipcdev; /* Only one for now */
>  
>  #define IPC_STATUS		0x04
> +#define IPC_STATUS_IRQ		BIT(2)
>  #define IPC_STATUS_ERR		BIT(1)
>  #define IPC_STATUS_BUSY		BIT(0)
>  
> @@ -107,11 +90,8 @@ static DEFINE_MUTEX(ipclock); /* lock used to prevent multiple call to SCU */
>   */
>  static inline void ipc_command(struct intel_scu_ipc_dev *scu, u32 cmd)
>  {
> -	if (scu->irq_mode) {
> -		reinit_completion(&scu->cmd_complete);
> -		writel(cmd | IPC_IOC, scu->ipc_base);
> -	}
> -	writel(cmd, scu->ipc_base);
> +	reinit_completion(&scu->cmd_complete);
> +	writel(cmd | IPC_IOC, scu->ipc_base);
>  }
>  
>  /*
> @@ -549,9 +529,10 @@ EXPORT_SYMBOL_GPL(intel_scu_ipc_raw_command);
>  static irqreturn_t ioc(int irq, void *dev_id)
>  {
>  	struct intel_scu_ipc_dev *scu = dev_id;
> +	int status = ipc_read_status(scu);
>  
> -	if (scu->irq_mode)
> -		complete(&scu->cmd_complete);
> +	writel(status | IPC_STATUS_IRQ, scu->ipc_base + IPC_STATUS);
> +	complete(&scu->cmd_complete);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -568,17 +549,10 @@ static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	int err;
>  	struct intel_scu_ipc_dev *scu = &ipcdev;
> -	struct intel_scu_ipc_pdata_t *pdata;
>  
>  	if (scu->dev)		/* We support only one SCU */
>  		return -EBUSY;
>  
> -	pdata = (struct intel_scu_ipc_pdata_t *)id->driver_data;
> -	if (!pdata)
> -		return -ENODEV;
> -
> -	scu->irq_mode = pdata->irq_mode;
> -
>  	err = pcim_enable_device(pdev);
>  	if (err)
>  		return err;
> @@ -605,13 +579,11 @@ static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	return 0;
>  }
>  
> -#define SCU_DEVICE(id, pdata)	{PCI_VDEVICE(INTEL, id), (kernel_ulong_t)&pdata}
> -
>  static const struct pci_device_id pci_ids[] = {
> -	SCU_DEVICE(PCI_DEVICE_ID_PENWELL,	intel_scu_ipc_penwell_pdata),
> -	SCU_DEVICE(PCI_DEVICE_ID_CLOVERVIEW,	intel_scu_ipc_penwell_pdata),
> -	SCU_DEVICE(PCI_DEVICE_ID_TANGIER,	intel_scu_ipc_tangier_pdata),
> -	{}
> +	{ PCI_VDEVICE(INTEL, 0x080e) },
> +	{ PCI_VDEVICE(INTEL, 0x08ea) },
> +	{ PCI_VDEVICE(INTEL, 0x11a0) },
> +	{ }
>  };
>  
>  static struct pci_driver ipc_driver = {
> -- 
> 2.24.1
> 

-- 
With Best Regards,
Andy Shevchenko