[alsa-devel] [PATCH v3 5/7] ASoC: intel - add Skylake HDA audio driver

[Vinod Koul]

On Wed, Apr 29, 2015 at 02:49:05PM +0200, Takashi Iwai wrote:
> At Wed, 29 Apr 2015 01:24:28 +0530,
> Vinod Koul wrote:
> > 
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_SUPPORTED_DEVICE("{Intel, PCH},");
> > +MODULE_DESCRIPTION("Intel aDSP HDA driver");
> 
> Are the descriptions correct?
What am i missing?

> 
> 
> > +/*
> > + * Check whether the current DMA position is acceptable for updating
> > + * periods.  Returns non-zero if it's OK.
> > + */
> > +static int azx_position_ok(struct hdac_bus *bus, struct hdac_stream *stream)
> > +{
> > +	u32 wallclk;
> > +	unsigned int pos = 0;
> > +
> > +	wallclk = snd_hdac_chip_readl(bus, WALLCLK) - stream->start_wallclk;
> > +	if (wallclk < (stream->period_wallclk * 2) / 3)
> > +		return -1;	/* bogus (too early) interrupt */
> > +
> > +	if (bus->use_posbuf) {
> > +		/* use the position buffer as default */
> > +		pos = snd_hdac_stream_get_pos_posbuf(stream);
> > +		if (!pos || pos == (u32)-1) {
> > +			dev_info(bus->dev,
> > +				 "Invalid pos buffer, using LPIB read method instead.\n");
> > +			pos = snd_hdac_stream_get_pos_lpib(stream);
> > +		}
> > +	}
> > +
> > +	if (pos >= stream->bufsize)
> > +		pos = 0;
> > +
> > +	if (WARN_ONCE(!stream->period_bytes,
> > +		      "hda-skl: zero stream->period_bytes"))
> > +		return -1; /* this shouldn't happen! */
> > +
> > +	if (wallclk < (stream->period_wallclk * 5) / 4 &&
> > +	    pos % stream->period_bytes > stream->period_bytes / 2)
> > +		/* NG - it's below the first next period boundary */
> > +		return bus->bdl_pos_adj ? 0 : -1;
> > +
> > +	stream->start_wallclk += wallclk;
> > +
> > +	return 1; /* OK, it's fine */
> 
> I'd love to drop this messy trick if SKL doesn't need it.
> Could you check whether it works without this?
Sure let me test this

> 
> 
> > +static irqreturn_t azx_interrupt(int irq, void *dev_id)
> > +{
> > +	struct soc_hdac_bus *sbus = dev_id;
> > +	struct hdac_bus *bus = hdac_bus(sbus);
> > +	u32 status;
> > +
> > +#ifdef CONFIG_PM
> > +	if (!pm_runtime_active(bus->dev))
> > +		return IRQ_NONE;
> > +#endif
> > +
> > +	spin_lock(&bus->reg_lock);
> > +
> > +	status = snd_hdac_chip_readl(bus, INTSTS);
> > +	if (status == 0 || status == 0xffffffff) {
> > +		spin_unlock(&bus->reg_lock);
> > +		return IRQ_NONE;
> > +	}
> > +
> > +	/* clear rirb int */
> > +	status = snd_hdac_chip_readb(bus, RIRBSTS);
> > +	if (status & RIRB_INT_MASK) {
> > +		if (status & RIRB_INT_RESPONSE)
> > +			snd_hdac_bus_update_rirb(bus);
> > +		snd_hdac_chip_writeb(bus, RIRBSTS, RIRB_INT_MASK);
> > +		return IRQ_HANDLED;
> 
> An irq may have both RIRB and stream irq bits set.  If we return
> IRQ_HANDLED here, I suppose the stream irqs won't be handled.
They will be in the thread handler. Also worth mentioning here is that the
aDSP interrupts will be handled by IPC code (shared interrupt)

Now yes we are missing fallback to coupled mode, if aDSP fails then stream
should be handled here, will add that

> 
> > +	}
> > +
> > +	spin_unlock(&bus->reg_lock);
> > +	return IRQ_WAKE_THREAD;
> > +}
> 
> The threaded irq handler checks only the stream irqs, so better to
> check INTSTS here.  That is, something like:
>   return snd_hdac_chip_readl(bus, INTSTS) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
Yes that makese sense

> 
> > +#ifdef CONFIG_PM_SLEEP
> > +/*
> > + * power management
> > + */
> > +static int azx_suspend(struct device *dev)
> > +{
> > +	struct pci_dev *pci = to_pci_dev(dev);
> > +	struct soc_hdac_bus *sbus = pci_get_drvdata(pci);
> > +	struct hdac_bus *bus = hdac_bus(sbus);
> > +	struct hda_skl *hda = to_hda_skl(sbus);
> > +
> > +	snd_hdac_bus_stop_chip(bus);
> > +	snd_hdac_bus_enter_link_reset(bus);
> > +	if (bus->irq >= 0) {
> > +		free_irq(bus->irq, bus);
> > +		bus->irq = -1;
> > +	}
> > +
> > +	if (hda->msi)
> > +		pci_disable_msi(pci);
> > +	pci_disable_device(pci);
> > +	pci_save_state(pci);
> > +	pci_set_power_state(pci, PCI_D3hot);
> 
> Drop the last three lines, they are done in PCI core nowadays.
ok

> 
> > +	return 0;
> > +}
> > +
> > +static int azx_resume(struct device *dev)
> > +{
> > +	struct pci_dev *pci = to_pci_dev(dev);
> > +	struct soc_hdac_bus *sbus = pci_get_drvdata(pci);
> > +	struct hdac_bus *bus = hdac_bus(sbus);
> > +	struct hda_skl *hda = to_hda_skl(sbus);
> > +
> > +	pci_set_power_state(pci, PCI_D0);
> > +	pci_restore_state(pci);
> > +
> > +	if (pci_enable_device(pci) < 0) {
> > +		dev_err(dev, "hda-intel: pci_enable_device failed, disabling device\n");
> > +		return -EIO;
> > +	}
> 
> These three calls should be dropped, too.
ok

> 
> 
> > +	pci_set_master(pci);
> > +	if (hda->msi)
> > +		if (pci_enable_msi(pci) < 0)
> > +			hda->msi = 0;
> > +	if (azx_acquire_irq(sbus, 1) < 0)
> > +		return -EIO;
> > +	azx_init_pci(hda);
> > +
> > +	snd_hdac_bus_init_chip(bus, 1);
> > +	return 0;
> > +}
> > +#endif /* CONFIG_PM_SLEEP */
> > +
> > +#ifdef CONFIG_PM
> > +static int azx_runtime_suspend(struct device *dev)
> > +{
> > +	struct pci_dev *pci = to_pci_dev(dev);
> > +	struct soc_hdac_bus *sbus = pci_get_drvdata(pci);
> > +	struct hdac_bus *bus = hdac_bus(sbus);
> > +
> > +	dev_dbg(bus->dev, "in %s\n", __func__);
> > +
> > +	/* enable controller wake up event */
> > +	snd_hdac_chip_updatew(bus, WAKEEN, 0, STATESTS_INT_MASK);
> > +
> > +	snd_hdac_bus_stop_chip(bus);
> > +	snd_hdac_bus_enter_link_reset(bus);
> > +	return 0;
> > +}
> > +
> > +static int azx_runtime_resume(struct device *dev)
> > +{
> > +	struct pci_dev *pci = to_pci_dev(dev);
> > +	struct soc_hdac_bus *sbus = pci_get_drvdata(pci);
> > +	struct hdac_bus *bus = hdac_bus(sbus);
> > +	struct hda_skl *hda = to_hda_skl(sbus);
> > +	int status;
> > +
> > +	dev_dbg(bus->dev, "in %s\n", __func__);
> > +
> > +	/* Read STATESTS before controller reset */
> > +	status = snd_hdac_chip_readw(bus, STATESTS);
> > +
> > +	azx_init_pci(hda);
> > +	snd_hdac_bus_init_chip(bus, true);
> > +	/* disable controller Wake Up event */
> > +	snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, 0);
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops azx_pm = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(azx_suspend, azx_resume)
> > +	SET_RUNTIME_PM_OPS(azx_runtime_suspend, azx_runtime_resume, NULL)
> > +};
> > +#endif /* CONFIG_PM */
> > +
> > +/*
> > + * destructor
> > + */
> > +static int azx_free(struct soc_hdac_bus *sbus)
> > +{
> > +	struct hda_skl *hda = to_hda_skl(sbus);
> > +	struct hdac_bus *bus = hdac_bus(sbus);
> > +
> > +	hda->init_failed = 1; /* to be sure */
> > +
> > +	snd_soc_hdac_stop_streams(sbus);
> > +
> > +	if (bus->irq >= 0)
> > +		free_irq(bus->irq, (void *)bus);
> > +	if (hda->msi)
> > +		pci_disable_msi(hda->pci);
> > +	if (bus->remap_addr)
> > +		iounmap(bus->remap_addr);
> > +
> > +	snd_hdac_bus_free_stream_pages(bus);
> > +	azx_free_streams(sbus);
> > +	pci_release_regions(hda->pci);
> > +	pci_disable_device(hda->pci);
> > +
> > +	snd_hdac_bus_exit(bus);
> > +	return 0;
> > +}
> > +
> > +static int hda_dmic_device_register(struct hda_skl *hda)
> > +{
> > +	struct hdac_bus *bus = hdac_bus(&hda->sbus);
> > +	struct platform_device *pdev;
> > +	int ret;
> > +
> > +	pdev = platform_device_alloc("dmic-codec", -1);
> > +	if (!pdev) {
> > +		dev_err(bus->dev, "failed to allocate dmic device\n");
> > +		return -1;
> > +	}
> > +
> > +	ret = platform_device_add(pdev);
> > +	if (ret) {
> > +		dev_err(bus->dev, "failed to add hda codec device\n");
> > +		platform_device_put(pdev);
> > +		return -1;
> > +	}
> > +	hda->dmic_dev = pdev;
> > +	return 0;
> > +}
> > +
> > +static void hda_dmic_device_unregister(struct hda_skl *hda)
> > +{
> > +
> > +	if (hda->dmic_dev)
> > +		platform_device_unregister(hda->dmic_dev);
> > +}
> > +
> > +static int azx_add_codec_device(int addr, struct hdac_bus *bus)
> > +{
> > +	struct hdac_device *hdev = NULL;
> > +	char name[10];
> > +	int ret;
> > +
> > +	hdev = devm_kzalloc(bus->dev, sizeof(*hdev), GFP_KERNEL);
> > +	if (!hdev)
> > +		return -ENOMEM;
> > +
> > +	snprintf(name, sizeof(name), "codec#%03x", addr);
> 
> The name should be unique, and this would conflict when there are
> multiple sound cards with the same codec address.  The legacy driver,
> for example, encodes the name with the card index and the codec
> address.
would we have multiple cards on a system with hda codecs? I am not too sure,
but yes lets add this

> 
> > +static int azx_first_init(struct soc_hdac_bus *sbus)
> > +{
> > +	struct hda_skl *hda = to_hda_skl(sbus);
> > +	struct hdac_bus *bus = hdac_bus(sbus);
> > +	struct pci_dev *pci = hda->pci;
> > +	int err;
> > +	unsigned short gcap;
> > +	int capture_streams, playback_streams;
> > +
> > +	err = pci_request_regions(pci, "ICH HD audio");
> 
> Choose a more sensible name for a new driver :)
Ah yes :) I am think "SKL HDA aDSP driver" or generically "HDA aDSP driver"


> 
> > +	if (err < 0)
> > +		return err;
> > +
> > +	bus->addr = pci_resource_start(pci, 0);
> > +	bus->remap_addr = pci_ioremap_bar(pci, 0);
> > +	if (bus->remap_addr == NULL) {
> > +		dev_err(bus->dev, "ioremap error\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	if (hda->msi)
> > +		if (pci_enable_msi(pci) < 0)
> > +			hda->msi = 0;
> > +
> > +	if (azx_acquire_irq(sbus, 0) < 0)
> > +		return -EBUSY;
> > +
> > +	pci_set_master(pci);
> > +	synchronize_irq(bus->irq);
> > +
> > +	gcap = snd_hdac_chip_readw(bus, GCAP);
> > +	dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap);
> > +
> > +	/* allow 64bit DMA address if supported by H/W */
> > +	if (!pci_set_dma_mask(pci, DMA_BIT_MASK(64)))
> > +		pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(64));
> > +	else {
> > +		pci_set_dma_mask(pci, DMA_BIT_MASK(32));
> > +		pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(32));
> > +	}
> 
> These are replaced with dma_set_mask().  See the codes in for-next
> branch.
Sure, will use that

> 
> > +/* PCI IDs */
> > +static const struct pci_device_id azx_ids[] = {
> > +	/* Sunrise Point-LP */
> > +	{ PCI_DEVICE(0x8086, 0x9d70), 0},
> > +	{ 0, }
> > +};
> > +MODULE_DEVICE_TABLE(pci, azx_ids);
> 
> We wanted to have a mechanism to work around the PCI ID conflict
> between asoc and legacy drivers.  Will it be included in the next
> series?
I will send that out first before this is merged, possiblu later today so
that you can see them before you leave :)

-- 
~Vinod