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