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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 12/11/18 4:57 PM, Andy Shevchenko wrote:
> 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()
Yes but no. This comes from firmware definitions, so it's easier to 
leave them alone. Changing the kernel side of the IPC for style reasons 
just adds more work and chances of divergence.
>
>> +#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() ?
same for all, when the definitions are shared with firmware it's better 
to keep the values as is.
>
>> +#define IPC_TIMEOUT_MSECS	300
> Simple _MS ?
yes.
>
>> +/* 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.
ok, nice catch.
>
>> +
>> +	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.
I believe they are intentional, but that's Liam's part.
>
>> +		 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");
That must be a result of me asking folks to run checkpatch.pl --strict...
>
>> +	}
>> +
>> +	/* 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.
ok
>
>> +
>> +	/* 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.
Dunno either, maybe something for Liam to look at?
>
>> +
>> +		init_waitqueue_head(&msg->waitq);
>> +		list_add(&msg->list, &ipc->empty_list);
>> +		msg++;
>> +	}
>> +
>> +	return ipc;
>> +}
>> +EXPORT_SYMBOL(snd_sof_ipc_init);