Re: [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Dec 11, 2018 at 03:23:08PM -0600, Pierre-Louis Bossart wrote:
> From: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> 
> Define an IPC ABI for all host <--> DSP communication. This ABI should
> be transport agnostic. i.e. it should work on MMIO and SPI/I2C style
> interfaces.

> + /* ssc1: TINTE */
> +#define SOF_DAI_INTEL_SSP_QUIRK_TINTE		(1 << 0)
> + /* ssc1: PINTE */
> +#define SOF_DAI_INTEL_SSP_QUIRK_PINTE		(1 << 1)
> + /* ssc2: SMTATF */
> +#define SOF_DAI_INTEL_SSP_QUIRK_SMTATF		(1 << 2)
> + /* ssc2: MMRATF */
> +#define SOF_DAI_INTEL_SSP_QUIRK_MMRATF		(1 << 3)
> + /* ssc2: PSPSTWFDFD */
> +#define SOF_DAI_INTEL_SSP_QUIRK_PSPSTWFDFD	(1 << 4)
> + /* ssc2: PSPSRWFDFD */
> +#define SOF_DAI_INTEL_SSP_QUIRK_PSPSRWFDFD	(1 << 5)

Style mismatch. Looks like a candidate for BIT()

> +#define SOF_DAI_FMT_FORMAT_MASK		0x000f
> +#define SOF_DAI_FMT_CLOCK_MASK		0x00f0
> +#define SOF_DAI_FMT_INV_MASK		0x0f00
> +#define SOF_DAI_FMT_MASTER_MASK		0xf000

GENMASK() ?

> +/* flags indicating which time stamps are in sync with each other */
> +#define	SOF_TIME_HOST_SYNC	(1 << 0)
> +#define	SOF_TIME_DAI_SYNC	(1 << 1)
> +#define	SOF_TIME_WALL_SYNC	(1 << 2)
> +#define	SOF_TIME_STAMP_SYNC	(1 << 3)
> +
> +/* flags indicating which time stamps are valid */
> +#define	SOF_TIME_HOST_VALID	(1 << 8)
> +#define	SOF_TIME_DAI_VALID	(1 << 9)
> +#define	SOF_TIME_WALL_VALID	(1 << 10)
> +#define	SOF_TIME_STAMP_VALID	(1 << 11)
> +
> +/* flags indicating time stamps are 64bit else 3use low 32bit */
> +#define	SOF_TIME_HOST_64	(1 << 16)
> +#define	SOF_TIME_DAI_64		(1 << 17)
> +#define	SOF_TIME_WALL_64	(1 << 18)
> +#define	SOF_TIME_STAMP_64	(1 << 19)

BIT() ?


> +#define SOF_IPC_PANIC_MAGIC_MASK		0x0ffff000
> +#define SOF_IPC_PANIC_CODE_MASK			0x00000fff

GENMASK() ?

> +#define IPC_TIMEOUT_MSECS	300

Simple _MS ?

> +/* find original TX message from DSP reply */
> +static struct snd_sof_ipc_msg *sof_ipc_reply_find_msg(struct snd_sof_ipc *ipc,
> +						      u32 header)
> +{
> +	struct snd_sof_dev *sdev = ipc->sdev;
> +	struct snd_sof_ipc_msg *msg;
> +
> +	header = SOF_IPC_MESSAGE_ID(header);
> +

> +	if (list_empty(&ipc->reply_list))
> +		goto err;

Redundant. list_for_each_*() have this check already.

> +
> +	list_for_each_entry(msg, &ipc->reply_list, list) {
> +		if (SOF_IPC_MESSAGE_ID(msg->header) == header)
> +			return msg;
> +	}
> +
> +err:
> +	dev_err(sdev->dev, "error: rx list empty but received 0x%x\n",
> +		header);
> +	return NULL;
> +}

> +int snd_sof_ipc_valid(struct snd_sof_dev *sdev)
> +{
> +	struct sof_ipc_fw_ready *ready = &sdev->fw_ready;
> +	struct sof_ipc_fw_version *v = &ready->version;
> +

> +	dev_info(sdev->dev,
> +		 " Firmware info: version %d:%d:%d-%s\n",  v->major, v->minor,

Indentation issues.

> +		 v->micro, v->tag);
> +	dev_info(sdev->dev,
> +		 " Firmware: ABI %d:%d:%d Kernel ABI %d:%d:%d\n",
> +		 SOF_ABI_VERSION_MAJOR(v->abi_version),
> +		 SOF_ABI_VERSION_MINOR(v->abi_version),
> +		 SOF_ABI_VERSION_PATCH(v->abi_version),
> +		 SOF_ABI_MAJOR, SOF_ABI_MINOR, SOF_ABI_PATCH);
> +
> +	if (SOF_ABI_VERSION_INCOMPATIBLE(SOF_ABI_VERSION, v->abi_version)) {
> +		dev_err(sdev->dev, "error: incompatible FW ABI version\n");
> +		return -EINVAL;
> +	}
> +
> +	if (ready->debug.bits.build) {
> +		dev_info(sdev->dev,
> +			 " Firmware debug build %d on %s-%s - options:\n"
> +			 "  GDB: %s\n"
> +			 "  lock debug: %s\n"
> +			 "  lock vdebug: %s\n",
> +			 v->build, v->date, v->time,
> +			 ready->debug.bits.gdb ? "enabled" : "disabled",
> +			 ready->debug.bits.locks ? "enabled" : "disabled",

> +			 ready->debug.bits.locks_verbose ?
> +			 "enabled" : "disabled");

I would leave it on one line (only 3 characters more, but better readability).

			 ready->debug.bits.locks_verbose ? "enabled" : "disabled");

> +	}
> +
> +	/* copy the fw_version into debugfs at first boot */
> +	memcpy(&sdev->fw_version, v, sizeof(*v));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(snd_sof_ipc_valid);

> +	dev_dbg(sdev->dev, "pre-allocate %d IPC messages\n",
> +		IPC_EMPTY_LIST_SIZE);

Noise.

> +
> +	/* pre-allocate message data */
> +	for (i = 0; i < IPC_EMPTY_LIST_SIZE; i++) {
> +		msg->msg_data = devm_kzalloc(sdev->dev, PAGE_SIZE, GFP_KERNEL);
> +		if (!msg->msg_data)
> +			return NULL;
> +
> +		msg->reply_data = devm_kzalloc(sdev->dev, PAGE_SIZE,
> +					       GFP_KERNEL);
> +		if (!msg->reply_data)
> +			return NULL;

Can't you just get enough (non-continuous?) pages at once via get_free_pages interface?
I didn't know that one, so, consider rather a way to look into.

> +
> +		init_waitqueue_head(&msg->waitq);
> +		list_add(&msg->list, &ipc->empty_list);
> +		msg++;
> +	}
> +
> +	return ipc;
> +}
> +EXPORT_SYMBOL(snd_sof_ipc_init);

-- 
With Best Regards,
Andy Shevchenko