On Tue, Dec 11, 2018 at 03:23:08PM -0600, Pierre-Louis Bossart wrote:
> From: Liam Girdwood <liam.r.girdwood@xxxxxxxxxxxxxxx>
>
> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
