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

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@intel.com Signed-off-by: Harsha Priya priya.harsha@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@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