Re: [PATCH 3/3] ipc: Added support for IPC interrupt mode

[One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>]

O> +	prompt "IPC access mode"
> +	depends on INTEL_SCU_IPC
> +	default INTEL_SCU_IPC_INTR_MODE
> +	---help---
> +	Select the desired access mode for IPC call.

This seems to depend at runtime on the platform so ifdefs seem
inappropriate.

>  static inline void ipc_command(u32 cmd) /* Send ipc command */
>  {
> +#ifdef CONFIG_INTEL_SCU_IPC_INTR_MODE
> +	INIT_COMPLETION(ipcdev.cmd_complete);
> +	writel(cmd | IPC_IOC, ipcdev.ipc_base);
> +#else
>  	writel(cmd, ipcdev.ipc_base);
> +#endif
>  }

If this is platform specific then add an IRQ to your platform data and
check for it then set an irq field in your scu objects and check at
runtime. If it depends upon the command and/or user then pass irq as a
parameter.

>  
>  /*
> @@ -194,6 +203,37 @@ static inline int busy_loop(void) /* Wait till scu status is busy */
>  	return 0;
>  }
>  
> +#ifdef CONFIG_INTEL_SCU_IPC_INTR_MODE
> +/* Wait till ipc ioc interrupt is received or timeout in 3 HZ */
> +static inline int ipc_wait_for_interrupt(void)
> +{
> +	int status;
> +	int ret = 0;
> +
> +	if (!wait_for_completion_timeout(&ipcdev.cmd_complete, 3 * HZ)) {
> +		ret = -ETIMEDOUT;
> +		goto end;
> +	}
> +
> +	status = ipc_read_status();
> +
> +	if ((status >> 1) & 1)
> +		ret = -EIO;
> +
> +end:
> +	return ret;

What happens on a timeout if the command then completes. Will it not
potentially produce a bogus completion on the next command just being
issued in some cases. Also it should probably be logged ?


So I think

1. Pass the informatio upon whether IRQ mode should be used in the
platform information and remove all the ifdeffery
2. Log timeouts
3. Explain what happens on a timeout that allows you to issue further
commands without a race - eg do you need to do any kind of reset or will
the next command issue stall sufficiently etc ?

Alan