[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