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

Andy Shevchenko
2018-12-11T22:57:33Z

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