[alsa-devel] [PATCH 4/7] ASoC: Intel: mrfld - add the dsp sst driver

[Vinod Koul]

On Fri, Jul 18, 2014 at 12:35:48PM +0100, Mark Brown wrote:
> On Wed, Jul 09, 2014 at 02:57:52PM +0530, Vinod Koul wrote:
> > The SST driver is the missing piece in our driver stack not upstreamed, so push
> > it now :) This driver currently supports PCI device on Merrifield. Future updates
> > will bring support for ACPI device as well as future update to PCI devices as
> > well
> 
> I've started reviewing this several times now but it's a really big
> change to review, and not just huge data tables but actual code.  It'd
> really benefit from being split up a bit further.
well I tried a lot of split up. But further split could have caused lots of
bisect issues. One way to do so was to add makefile last. I can try that
next time if you are fine
> 
> Have you looked at the mailbox framework (not merged yet but hopefully
> getting close to it)?
> 
> > +MODULE_VERSION(SST_DRIVER_VERSION);
> 
> Probably better to drop driver versions from mainline.
Sure thing!

> 
> > +#define SST_IS_PROCESS_REPLY(header) ((header & PROCESS_MSG) ? true : false)
> > +#define SST_VALIDATE_MAILBOX_SIZE(size) ((size <= SST_MAILBOX_SIZE) ? true : false)
> 
> Inline functions please.
> 
> > +static irqreturn_t intel_sst_irq_thread_mrfld(int irq, void *context)
> > +{
> > +	struct intel_sst_drv *drv = (struct intel_sst_drv *) context;
> > +	struct ipc_post *__msg, *msg = NULL;
> > +	unsigned long irq_flags;
> > +
> > +	if (list_empty(&drv->rx_list))
> > +		return IRQ_HANDLED;
> 
> Why would the IRQ thread be running with nothing to do, and if it can
> be...
Cant think of, we need to remove this!

> 
> > +	spin_lock_irqsave(&drv->rx_msg_lock, irq_flags);
> > +	list_for_each_entry_safe(msg, __msg, &drv->rx_list, node) {
> 
> ...won't we handle that gracefully anyway?
Are you referring to lock + irqsave?

> > +int sst_alloc_drv_context(struct device *dev)
> > +{
> > +	struct intel_sst_drv *ctx;
> > +
> > +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx) {
> > +		pr_err("malloc fail\n");
> > +		return -ENOMEM;
> > +	}
> > +	sst_drv_ctx = ctx;
> > +	return 0;
> > +}
> 
> As others were saying I'd really like to see some sort of case being
> made for a global variable, and if we really do need a global variable
> why not just declare it rather than dynamically allocating?
For this case none, we can try removing this which was more for legacy
reasons when we had pvt ioctls for offload

> > +	if (0 != sst_driver_ops(sst_drv_ctx))
> > +		return -EINVAL;
> 
> if (!sst_driver_ops())
> 
> > +#define SST_DRIVER_VERSION "5.0.0"
> 
> Generally better to drop these from mainline, the kernel version should
> be enough.
> 
> > +static inline int sst_pm_runtime_put(struct intel_sst_drv *sst_drv)
> > +{
> > +	int ret;
> > +
> > +	ret = pm_runtime_put_sync(sst_drv->dev);
> > +	if (ret < 0)
> > +		return ret;
> > +	atomic_dec(&sst_drv->pm_usage_count);
> > +
> > +	pr_debug("%s: count is %d now..\n", __func__,
> > +			atomic_read(&sst_drv->pm_usage_count));
> > +	return 0;
> > +}
> 
> I'm not entirely clear why this is here but if it's to provide trace
> just instrument the runtime PM core.
Well the only usage of pm_usage_count locally is to check in runtime pm
suspend to S3 transitions. When we are runtime suspended and S3 triggers,
the device usage count goes up to 1. Then runtime resume and classic suspend
is invoked in that order. So if we are in this scenario we can't rely on
checks using frameworks count.
> 
> > + * @sst_drv_ctx : driver context
> > + *
> > + * this inline function assigns a private id for calls that dont have stream
> > + * context yet, should be called with lock held
> > + */
> > +static inline unsigned int sst_assign_pvt_id(struct intel_sst_drv *sst_drv_ctx)
> > +{
> > +	unsigned int local;
> > +
> > +	spin_lock(&sst_drv_ctx->block_lock);
> > +	sst_drv_ctx->pvt_id++;
> > +	if (sst_drv_ctx->pvt_id > MAX_BLOCKS)
> > +		sst_drv_ctx->pvt_id = 1;
> > +	local = sst_drv_ctx->pvt_id;
> > +	spin_unlock(&sst_drv_ctx->block_lock);
> > +	return local;
> > +}
> 
> I would expect this to be checking to see if the IDs are free but it
> just hands them out in sequence?  There appears to be an array of
> streams statically allocated, why not look there for an unused one?
we use these incrementally. The driver stamps each IPC to DSP with a number.
The size is limited by bits in the IPCs. So we keep going till we hit max,
that time we set it back to 1

These ids are not freed or allocated as IPC latency is small enough and IPCs
less. We haven't hot scenarios where we go complete round but IPC is still
pending :)

> 
> > +static inline void
> > +sst_set_fw_state_locked(struct intel_sst_drv *sst_drv_ctx, int sst_state)
> > +{
> > +	mutex_lock(&sst_drv_ctx->sst_lock);
> > +	sst_drv_ctx->sst_state = sst_state;
> > +	mutex_unlock(&sst_drv_ctx->sst_lock);
> > +}
> 
> I can't see any references to this in the code which is good as it seems
> a little odd to have a function like this.
My bad, this was to be removed :(

> > +int register_sst(struct device *);
> > +int unregister_sst(struct device *);
> 
> Everything else is sst_
Hmmm fair point!
> 
> > +int sst_download_fw(void)
> > +{
> > +	int retval = 0;
> > +
> > +	retval = sst_load_fw();
> > +	if (retval)
> > +		return retval;
> > +	pr_debug("fw loaded successful!!!\n");
> > +
> > +	if (sst_drv_ctx->ops->restore_dsp_context)
> > +		sst_drv_ctx->ops->restore_dsp_context();
> > +	sst_drv_ctx->sst_state = SST_FW_RUNNING;
> > +	return retval;
> > +}
> 
> So sst_load_fw() is wrapped with this function sst_download_fw() which
> does a tiny bit of extra stuff which might be mostly better done in the
> core function...
ok

> 
> > +int free_stream_context(unsigned int str_id)
> > +{
> > +	struct stream_info *stream;
> > +	int ret = 0;
> > +
> > +	stream = get_stream_info(str_id);
> > +	if (stream) {
> 
> It's a bit scary that this might be used with an invalid ID - what
> happens if the ID got reallocated?
The free routine checks for valid ID first

> 
> > +	retval = sst_drv_ctx->ops->alloc_stream((char *) str_param, block);
> > +	str_id = retval;
> 
> This cast is scary...  should alloc_stream() be taking a void?
Well we had two versions die to tow gens of DSP FW, we mught be able to
clean this up, will give it a shot!

> 
> > +	/* Block the call for reply */
> > +	retval = sst_wait_timeout(sst_drv_ctx, block);
> > +	if (block->data) {
> 
> > +	} else if (retval != 0) {
> 
> It'd be less surprising if the first check were for retval.
ok

> 
> > +	pr_debug("In %s\n", __func__);
> > +	/* stream is not allocated, we are allocating */
> > +	retval = sst_get_stream_allocated(str_param, &lib_dnld);
> > +
> > +	if  (retval <= 0) {
> 
> Coding style (there's quite a bit of odd coding styles throughout this
> code, slightly odd indentation, missing blanks and so on - it generally
> doesn't quite look like Linux code).
well this was started off in 2008, never got to a point to push upstream
though tried few times!. Will fix these
> 
> > +/**
> > +* intel_sst_check_device - checks SST device
> > +*
> > +* This utility function checks the state of SST device and downlaods FW if
> > +* not done, or resumes the device if suspended
> > +*/
> > +int intel_sst_check_device(void)
> 
> Can we have a better name please?  But this all seems very odd, why is
> runtime PM not causing the firmware to be loaded?
well as previously stated runtime resume might be invoked during S3, so we
wnat to load fw after calling _get_sync() so rather than calling _get_sync()
and download everywhere we wrapped it here
> 
> > +	/* The free_stream will return a error if there is no stream to free,
> > +	(i.e. the alloc failure case). And in this case the open does a put in
> > +	the error scenario, so skip in this case.
> > +		In the close we need to handle put in the success scenario and
> > +	the timeout error(EBUSY) scenario. */
> 
> Another weird coding style thing.
will fix

> 
> > +static int sst_device_control(int cmd, void *arg)
> > +{
> > +	int retval = 0, str_id = 0;
> > +
> > +	if (sst_drv_ctx->sst_state != SST_FW_RUNNING)
> > +		return 0;
> > +
> > +	switch (cmd) {
> > +	case SST_SND_START: {
> 
> Eew, ioctl().  This would be clearer if it were a series of functions,
> there's nothing shared here.
> 
> > +static int sst_set_generic_params(enum sst_controls cmd, void *arg)
> > +{
> > +	int ret_val = 0;
> > +
> > +	if (NULL == arg)
> > +		return -EINVAL;
> 
> Again an ioctl()...
okay will move to series of functions

> 
> > +	/* check busy bit */
> > +	header.full = sst_shim_read64(sst_drv_ctx->shim, SST_IPCX);
> > +	while (header.p.header_high.part.busy) {
> > +		if (loop_count > 10) {
> > +			pr_err("sst: Busy wait failed, cant send this msg\n");
> > +			retval = -EBUSY;
> > +			goto out;
> > +		}
> > +		udelay(500);
> > +		loop_count++;
> > +		header.full = sst_shim_read64(sst_drv_ctx->shim, SST_IPCX);
> > +	}
> 
> Throw a cpu_relax() in there if we need to spin...
its more busy wait for the bit to be freed. Since IPC latency is supposed to
be minimal checking like this helps to minimize.

> 
> > +	pr_info("FW Version %02x.%02x.%02x.%02x\n",
> > +		init->fw_version.type, init->fw_version.major,
> > +		init->fw_version.minor, init->fw_version.build);
> > +	pr_info("Build date %s Time %s\n",
> > +			init->build_info.date, init->build_info.time);
> 
> Same comment as for some of the other firmwares - consider providing
> this via /proc or sysfs as well.
ok

> 
> > +static inline int sst_validate_elf(const struct firmware *sst_bin, bool dynamic)
> > +{
> > +	Elf32_Ehdr *elf;
> > +
> > +	BUG_ON(!sst_bin);
> > +
> > +	pr_debug("IN %s\n", __func__);
> > +
> > +	elf = (Elf32_Ehdr *)sst_bin->data;
> > +
> > +	if ((elf->e_ident[0] != 0x7F) || (elf->e_ident[1] != 'E') ||
> > +	    (elf->e_ident[2] != 'L') || (elf->e_ident[3] != 'F')) {
> > +		pr_debug("ELF Header Not found!%zu\n", sst_bin->size);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (dynamic == true) {
> > +		if (elf->e_type != ET_DYN) {
> > +			pr_err("Not a dynamic loadable library\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +	pr_debug("Valid ELF Header...%zu\n", sst_bin->size);
> > +	return 0;
> > +}
> 
> We have generic ELF handling code in the kernel, should this go there?
will check that

> 
> > +{
> > +	struct fw_header *header;
> 
> struct fw_header should probably be better namespaced?
> 
> > +void sst_memcpy_free_resources(void)
> > +{
> > +	struct sst_memcpy_list *listnode, *tmplistnode;
> > +
> > +	pr_debug("entry:%s\n", __func__);
> > +
> > +	/*Free the list*/
> > +	if (!list_empty(&sst_drv_ctx->memcpy_list)) {
> > +		list_for_each_entry_safe(listnode, tmplistnode,
> > +				&sst_drv_ctx->memcpy_list, memcpylist) {
> > +			list_del(&listnode->memcpylist);
> > +			kfree(listnode);
> > +		}
> > +	}
> > +	sst_memcpy_free_lib_resources();
> > +}
> 
> I'm having a hard time seeing why we don't just copy the data as we go
> rather than allocating this list?  It seems to just be making the code
> more complex.
because we support DMA too. so common parsing and then later we either do
memcpy and dma. Will push DMA bits after this series.

> > +/**
> > + * sst_next_track: notify next track
> > + * @str_id:		stream ID
> > + *
> > + * This function is called by any function which wants to
> > + * set next track. Current this is NOP as FW doest care
> > + */
> > +int sst_next_track(void)
> > +{
> > +	pr_debug("SST DBG: next_track");
> > +	return 0;
> > +}
> 
> Shouldn't the operation just be made optional then, obviously the upper
> layers don't care either?
Sure


-- 
~Vinod
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140718/71b04389/attachment-0001.sig>