[alsa-devel] [PATCH] [RFC 2/13] Intel SST driver

Takashi Iwai tiwai at suse.de
Fri Jul 3 12:51:35 CEST 2009


At Fri,  3 Jul 2009 12:31:33 +0530,
Vinod Koul wrote:
> 
> This adds the SST driver for the SST DSP engine. This patch
> adds the main file intel_sst.c which contains the init, exit
> probe, interrupt routine and PCI suspend/resume implementation.
> This file also implements the SST Firmware initialization and
> firmware and codec libraries download steps

The patch should be builtable.  So, if you need to split a patch
to several files, the patch for header files should be before the
body.

> 
> Signed-off-by: Vinod Koul <vinod.koul at intel.com>
> Signed-off-by: Harsha Priya <priya.harsha at intel.com>
> 
> 	new file:   sound/pci/sst/intel_sst.c
> ---
>  sound/pci/sst/intel_sst.c |  877 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 877 insertions(+), 0 deletions(-)
>  create mode 100644 sound/pci/sst/intel_sst.c
> 
> diff --git a/sound/pci/sst/intel_sst.c b/sound/pci/sst/intel_sst.c
> new file mode 100644
> index 0000000..bd30833
> --- /dev/null
> +++ b/sound/pci/sst/intel_sst.c
(snip)
> +#ifndef CONFIG_SST_IPC_NOT_INCLUDED
> +#include <asm/ipc_defs.h>
> +#endif

What is this file?

> +#include <sound/intel_sst.h>
> +#include <sound/intel_sst_ioctl.h>
> +#include "intel_sst_fw_ipc.h"
> +#include "intel_sst_common.h"
> +#include "intel_sst_pvt.h"
> +#ifdef CONFIG_SST_OSPM_SUPPORT
> +#include <linux/intel_mid.h>
> +#endif

The include files shouldn't be much depending on kconfig.
If any kconfig-dependent stuff is present, define the inline dummy
functions (just returning an error or so) with ifdef in the header
file.

> +MODULE_AUTHOR("Vinod Koul <vinod.koul at intel.com>");
> +MODULE_DESCRIPTION("Intel (R) Moorestown Audio Engine Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION(SST_DRIVER_VERSION);
> +
> +struct intel_sst_drv *sst_ops;

I'm afraid it's a bit too generic name.

> +void sst_reset(void)

Shouldn't be static?

> +{
> +	int retval;
> +
> +	/*Disable Audio Core clock and Audio Fabric clock*/

Put a space around /* and */ (unless the rest space is too tight).
It's not mandatory but spaces there would increase readability.

> +	retval = sst_scu_ipc_write(0xff11d83c, 0x80008008);

Please avoid magic numbers but define them.

> +	if (retval != 0)
> +		sst_err("scu ipc write1 failed %d", retval);
> +	/*Reset the Audio subsystem*/
> +	retval = sst_scu_ipc_write(0xff11d118, 0x7ffffcff);
> +	if (retval != 0)
> +		sst_err("scu ipc write2 failed %d", retval);
> +	/*Bring it out of Audio subsystem reset*/
> +	retval = sst_scu_ipc_write(0xff11d118, 0x7fffffff);
> +	if (retval != 0)
> +		sst_err("scu ipc write3 failed %d", retval);
> +	/*Enable fabric clock at 50MHz*/
> +	retval = sst_scu_ipc_write(0xff11d83c, 0x80008301);
> +	if (retval != 0)
> +		sst_err("scu ipc write4 failed %d", retval);
> +	/*Write to the Shim register for ratio 1:1*/
> +	retval = sst_scu_ipc_write(0xffae8000, 0x382);
> +	if (retval != 0)
> +		sst_err("scu ipc write5 failed %d", retval);
> +	/*Enable the core clock at 1:2*/
> +	retval = sst_scu_ipc_write(0xff11d83c, 0x80000301);
> +	if (retval != 0)
> +		sst_err("scu ipc write6 failed %d", retval);
> +	return;

So... no these errors are no critical errors?

> +}
> +
> +void sst_start(void)

Missing static?

> +{
> +	union config_status_reg csr;
> +	int retval;
> +
> +	csr.full = readl(sst_ops->shim + SST_CSR);
> +	csr.part.sst_reset = 0;
> +	csr.part.run_stall = 0;
> +	csr.part.bypass = 0;
> +	/*csr.full = 0x1800;*/

I guess the way using union isn't portable.
Will write later in the header file patch.


> +	sst_dbg("Setting SST to execute 0x%x \n", csr.full);
> +
> +	retval = sst_scu_ipc_write(sst_ops->shim_phy_add + SST_CSR, csr.full);
> +	if (retval != 0)
> +		sst_err("scu ipc write start failed %d", retval);
> +	return;
> +}

Missing a blank line.

> +int sst_parse_module(struct fw_module_header *module)

Hmm... are all these global?

> +{
> +	struct dma_block_info *block = NULL;

Superfluous initialization.
Similar lines appear in the later, too.

> +int sst_load_library(struct snd_sst_lib_download *lib, u8 ops, u32 pvt_id)
> +{
> +	char buf[20];
> +	int len = 0, error = 0;
> +	u32 entry_point;
> +	const struct firmware *fw_lib;
> +	struct snd_sst_lib_download_info dload_info = {{{0},},};
> +
> +	memset(buf, 0, sizeof(buf));
> +
> +	sst_dbg("Lib Type 0x%x, Slot 0x%x, ops 0x%x \n",
> +			lib->lib_info.lib_type, lib->slot_info.slot_num, ops);
> +	sst_dbg("Version 0x%x, name %s, caps 0x%x media type 0x%x \n",
> +		lib->lib_info.lib_version, lib->lib_info.lib_name,
> +		lib->lib_info.lib_caps, lib->lib_info.media_type);
> +
> +	sst_dbg("IRAM Size 0x%x, offset 0x%x, DRAM Size 0x%x, offset 0x%x \n",
> +		lib->slot_info.iram_size, lib->slot_info.iram_offset,
> +		lib->slot_info.dram_size, lib->slot_info.dram_offset);
> +
> +	switch (lib->lib_info.lib_type) {
> +	case SST_CODEC_TYPE_MP3:
> +		len += snprintf(buf + len, sizeof(buf) - len, "mp3_");
> +		break;
> +	case SST_CODEC_TYPE_AAC:
> +		len += snprintf(buf + len, sizeof(buf) - len, "aac_");
> +		break;
> +	case SST_CODEC_TYPE_WMA9:
> +		len += snprintf(buf + len, sizeof(buf) - len, "wma9_");
> +		break;
> +	default:
> +		sst_err("Invalid codec type \n");
> +		error = -EINVAL;
> +		goto wake;
> +	}
> +
> +	if (ops == STREAM_OPS_CAPTURE)
> +		len += snprintf(buf + len, sizeof(buf) - len, "enc_");
> +	else
> +		len += snprintf(buf + len, sizeof(buf) - len, "dec_");
> +
> +	len += snprintf(buf + len, sizeof(buf) - len, "%d",
> +			lib->slot_info.slot_num);
> +	len += snprintf(buf + len, sizeof(buf) - len, ".bin");

It would be simpler in a way like:

	char buf[20];
	const char *type;
	const char *dir; 

	switch (lib->lib_info.lib_type) {
	case SST_CODEC_TYPE_MP3:
		type = "mp3";
		break;
	case SST_CODEC_TYPE_AAC:
		type = "aac";
		break;
	case SST_CODEC_TYPE_WMA9:
		type = "wma9";
		break;
	default:
		sst_err("Invalid codec type \n");
		error = -EINVAL;
		goto wake;
	}

	if (ops == STREAM_OPS_CAPTURE)
		dir = "enc";		
	else
		dir = "dec";

	snprintf(buf, sizeof(buf), "%s_%s_%d.bin",
		 type, dir, lib->slot_info.slot_num);


> +/*fops Routines*/
> +const struct file_operations intel_sst_fops = {

Shouldn't be statc?

> +
> +/*
> +	ISR Routines
> +*/
> +/**
> +* intel_sst_interrupt - Interrupt service routine for SST
> +* @irq:		irq number of interrupt
> +* @dev_id:	pointer to device structre
> +*
> +* This function is called by OS when SST device raises
> +* an interrupt. This will be result of write in IPC register
> +* Source can be busy or done interrupt
> +*/
> +static irqreturn_t intel_sst_interrupt(int irq, void *context)
> +{
> +	union interrupt_reg 	isr;
> +	union ipc_header 	header;
> +	union  interrupt_reg imr;
> +	struct intel_sst_drv *drv = (struct intel_sst_drv *) context;
> +	unsigned int size = 0;
> +
> +	/*Interrupt arrived, check src*/
> +	isr.full = readl(drv->shim + SST_ISRX);
> +	imr.full = readl(drv->shim + SST_IMRX);
> +	if (1 == isr.part.busy_interrupt) {
> +		header.full = readl(drv->shim + SST_IPCD);
> +		if (1 == header.part.large)
> +			size = header.part.data;
> +		if (0 != (header.part.msg_id & REPLY_MSG)) {
> +			sst_ops->ipc_process_msg.header = header;
> +			memcpy_fromio(sst_ops->ipc_process_msg.mailbox,
> +				drv->mailbox + SST_MAILBOX_RCV, size);
> +			schedule_work(&sst_ops->ipc_process_msg.wq);
> +		} else {
> +			sst_ops->ipc_process_reply.header = header;
> +			memcpy_fromio(sst_ops->ipc_process_reply.mailbox,
> +				drv->mailbox + SST_MAILBOX_RCV, size);
> +			schedule_work(&sst_ops->ipc_process_reply.wq);
> +		}
> +		/*mask busy inetrrupt*/
> +		imr.part.busy_interrupt = 1;
> +		writel(imr.full, drv->shim + SST_IMRX);
> +
> +	} else if (1 == isr.part.done_interrupt) {
> +		/*Clear done bit*/
> +		header.full = readl(drv->shim + SST_IPCX);
> +		header.part.done = 0;
> +		writel(header.full, drv->shim + SST_IPCX);
> +		/* write 1 to clear status register*/;
> +		isr.part.done_interrupt = 1;
> +		writel(isr.full, drv->shim + SST_ISRX);
> +		schedule_work(&sst_ops->ipc_post_msg.wq);
> +	}
> +	return IRQ_HANDLED;

There is no IRQ_NONE.  It can't work with a shared irq.

> +static int __devinit intel_sst_probe(struct pci_dev *pci,
> +			const struct pci_device_id *pci_id)
> +{
> +	int ret = 0;
> +	u32 bar = 0, size = 0;
> +
> +	/*Init the device*/
> +	ret = pci_enable_device(pci);
> +	if (0 != ret) {
> +		sst_err("device cant be enabled\n");
> +		return ret;
> +	}
> +	sst_ops->pci = pci;
> +	/*map registers*/
> +	/*SST Shim*/
> +	bar = pci_resource_start(pci, 1);
> +	size = pci_resource_len(pci, 1);
> +	ret = pci_request_region(pci, 2, SST_DRV_NAME);
> +	if (ret != 0)
> +		goto err_1;
> +	sst_ops->shim_phy_add = bar;
> +	sst_ops->shim = ioremap_nocache(bar, size);
> +	if (sst_ops->shim == NULL)
> +		goto err_2;
> +	sst_dbg("SST Shim 0x%x,Size 0x%x,Ptr %p \n", bar, size, sst_ops->shim);
> +
> +	/*Shared SRAM*/
> +	bar = pci_resource_start(pci, 2);
> +	size = pci_resource_len(pci, 2);
> +	ret = pci_request_region(pci, 3, SST_DRV_NAME);
> +	if (ret != 0)
> +		goto err_2;

It's easier to use pci_request_regions().  Then you'll get all regions
at once.  Also, pci_ioremap_bar() is a newer function to do ioremapping.
It's more recommended, at least, for new drivers.

> +#ifdef CONFIG_SST_OSPM_SUPPORT

Can't be simple CONFIG_PM?


Takashi


More information about the Alsa-devel mailing list