From 8c4b558f80994275e5f465ef6886c13d5a03c81e Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 12 Nov 2018 16:03:19 -0600 Subject: [PATCH 01/10] ASoC: SOF: Intel: hda: fix error handling in probe The GPROCEN bit is only set when there is no error path, so don't clear it on all errors. Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 8ef9484ee0068d..9253168e76597f 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -721,9 +721,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) free_streams: hda_dsp_stream_free(sdev); err: - /* disable DSP */ - snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL, - SOF_HDA_PPCTL_GPROCEN, 0); return ret; } From 36bc5097bb9ad19913cf2957400da451fe310b7f Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 12 Nov 2018 16:10:33 -0600 Subject: [PATCH 02/10] ASoC: SOF: Intel: hda: fix sof_hda_bus_init This function returns zero always, change return type to void Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda-bus.c | 7 ++----- sound/soc/sof/intel/hda.h | 4 ++-- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c index 759f1e61a0309c..e7fd07f1f80d5b 100644 --- a/sound/soc/sof/intel/hda-bus.c +++ b/sound/soc/sof/intel/hda-bus.c @@ -75,10 +75,9 @@ static const struct hdac_io_ops io_ops = { /* * This can be used for both with/without hda link support. - * Returns 0 if successful, or a negative error code. */ -int sof_hda_bus_init(struct hdac_bus *bus, struct device *dev, - const struct hdac_ext_bus_ops *ext_ops) +void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev, + const struct hdac_ext_bus_ops *ext_ops) { static int idx; @@ -103,6 +102,4 @@ int sof_hda_bus_init(struct hdac_bus *bus, struct device *dev, mutex_init(&bus->lock); bus->cmd_dma_state = true; - - return 0; } diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index a1a525376d760f..f31c1b02eb394e 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -504,8 +504,8 @@ int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev, bool full_reset); /* * HDA bus operations. */ -int sof_hda_bus_init(struct hdac_bus *bus, struct device *dev, - const struct hdac_ext_bus_ops *ext_ops); +void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev, + const struct hdac_ext_bus_ops *ext_ops); #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) /* From 06f24028f240084e9f9ce6dd76693fa096729f0f Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 12 Nov 2018 16:20:44 -0600 Subject: [PATCH 03/10] ASoC: SOF: Intel: hda: unmap on hda_init error Add new goto to handle possible errors and unmap bar Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 9253168e76597f..eda53d7e00d8ea 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -563,9 +563,10 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) #endif /* set up HDA base */ + bus = sof_to_bus(sdev); ret = hda_init(sdev); if (ret < 0) - return ret; + goto hdac_bus_unmap; /* DSP base */ sdev->bar[HDA_DSP_BAR] = pci_ioremap_bar(pci, HDA_DSP_BAR); @@ -619,7 +620,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) sdev->ipc_irq = sdev->hda->irq; } - bus = sof_to_bus(sdev); dev_dbg(sdev->dev, "using HDA IRQ %d\n", sdev->hda->irq); ret = request_threaded_irq(sdev->hda->irq, hda_dsp_stream_interrupt, hda_dsp_stream_threaded_handler, @@ -720,6 +720,8 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) pci_free_irq_vectors(pci); free_streams: hda_dsp_stream_free(sdev); +hdac_bus_unmap: + iounmap(bus->remap_addr); err: return ret; } From 90277c34643cb7d9265cbeaebb442ebbabcb2697 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 12 Nov 2018 16:22:21 -0600 Subject: [PATCH 04/10] ASoC: SOF: Intel: hda: handle error on DSP BAR remap Jump to correct goto instead of just returning -ENXIO Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index eda53d7e00d8ea..22932eb2f25915 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -572,7 +572,8 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) sdev->bar[HDA_DSP_BAR] = pci_ioremap_bar(pci, HDA_DSP_BAR); if (!sdev->bar[HDA_DSP_BAR]) { dev_err(&pci->dev, "error: ioremap error\n"); - return -ENXIO; + ret = -ENXIO; + goto hdac_bus_unmap; } sdev->mmio_bar = HDA_DSP_BAR; From 304a296ec8df298214594e197304b8127ffbab90 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 12 Nov 2018 16:36:47 -0600 Subject: [PATCH 05/10] ASoC: SOF: Intel: hda: free streams on error The comments make no sense, the streams are not freed on error. Fix. Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 22932eb2f25915..74791786943184 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -597,7 +597,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) * not all errors are due to memory issues, but trying * to free everything does not harm */ - goto err; + goto free_streams; } /* From 52e0435ef4f744e41284b9c54d29ec40605a6309 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 12 Nov 2018 16:40:42 -0600 Subject: [PATCH 06/10] ASoC: SOF: hda: Intel: free dsp bar mapping Add label (not currently used) which is always reached with free_streams Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 74791786943184..cc232e176d2d8a 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -721,6 +721,8 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) pci_free_irq_vectors(pci); free_streams: hda_dsp_stream_free(sdev); +/* dsp_unmap: not currently used */ + iounmap(sdev->bar[HDA_DSP_BAR]); hdac_bus_unmap: iounmap(bus->remap_addr); err: From bdfd5693ea530c8d304ced04bdfa248555f393f2 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 12 Nov 2018 16:49:37 -0600 Subject: [PATCH 07/10] ASoC: SOF: hda: be consistent and free hda->irq instead of pci->irq Follow Keyon'd recommendation Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index cc232e176d2d8a..ddb803dc074fb6 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -758,7 +758,7 @@ int hda_dsp_remove(struct snd_sof_dev *sdev) SOF_HDA_PPCTL_GPROCEN, 0); free_irq(sdev->ipc_irq, sdev); - free_irq(sdev->pci->irq, bus); + free_irq(sdev->hda->irq, bus); pci_free_irq_vectors(pci); hda_dsp_stream_free(sdev); From aeb1db37fc7790608f26390a5e881b3c99491d9e Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 12 Nov 2018 16:51:28 -0600 Subject: [PATCH 08/10] ASoC: SOF: Intel: hda: unmap on remove Follow same pattern as for error handling on probe Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index ddb803dc074fb6..d4ffdb2a76629d 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -762,6 +762,10 @@ int hda_dsp_remove(struct snd_sof_dev *sdev) pci_free_irq_vectors(pci); hda_dsp_stream_free(sdev); + + iounmap(sdev->bar[HDA_DSP_BAR]); + iounmap(bus->remap_addr); + return 0; } From dc91aeae21a138e8ff7e59ad8fa977aee669a351 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 12 Nov 2018 16:01:38 -0600 Subject: [PATCH 09/10] ASoC: SOF: Intel: clean-up i915 support Add missing exit and get/put helpers. It's assumed that the device will not be active on exit - so we don't need to decrease the refcount. Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda-codec.c | 49 +++++++++++++++++++++++++++++++-- sound/soc/sof/intel/hda-dsp.c | 4 +-- sound/soc/sof/intel/hda.c | 13 +++++---- sound/soc/sof/intel/hda.h | 8 +++++- 4 files changed, 62 insertions(+), 12 deletions(-) diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index fbf64635b309e6..9701aac357b3ea 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -112,6 +112,34 @@ int hda_codec_probe_bus(struct snd_sof_dev *sdev) } EXPORT_SYMBOL(hda_codec_probe_bus); +int hda_codec_i915_get(struct snd_sof_dev *sdev) +{ + struct hdac_bus *bus = sof_to_bus(sdev); + int ret; + + dev_dbg(bus->dev, "Turning i915 HDAC power on\n"); + ret = snd_hdac_display_power(bus, true); + if (ret < 0) + dev_err(bus->dev, "i915 HDAC power on failed %d\n", ret); + + return ret; +} +EXPORT_SYMBOL(hda_codec_i915_get); + +int hda_codec_i915_put(struct snd_sof_dev *sdev) +{ + struct hdac_bus *bus = sof_to_bus(sdev); + int ret; + + dev_dbg(bus->dev, "Turning i915 HDAC power off\n"); + ret = snd_hdac_display_power(bus, false); + if (ret < 0) + dev_err(bus->dev, "i915 HDAC power off failed %d\n", ret); + + return ret; +} +EXPORT_SYMBOL(hda_codec_i915_put); + int hda_codec_i915_init(struct snd_sof_dev *sdev) { struct hdac_bus *bus = sof_to_bus(sdev); @@ -122,12 +150,27 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) if (ret < 0) return ret; - ret = snd_hdac_display_power(bus, true); - if (ret < 0) - dev_err(bus->dev, "i915 HDAC power on failed %d\n", ret); + ret = hda_codec_i915_get(sdev); return ret; } EXPORT_SYMBOL(hda_codec_i915_init); +int hda_codec_i915_exit(struct snd_sof_dev *sdev) +{ + struct hdac_bus *bus = sof_to_bus(sdev); + int ret; + + /* + * we don't need to decrease the refcount with + * hda_codec_i915_put() on exit since the device cannot be + * active + */ + + ret = snd_hdac_i915_exit(bus); + + return ret; +} +EXPORT_SYMBOL(hda_codec_i915_exit); + MODULE_LICENSE("Dual BSD/GPL"); diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c index 4d7028ff7a9c09..9e1874f3f4d268 100644 --- a/sound/soc/sof/intel/hda-dsp.c +++ b/sound/soc/sof/intel/hda-dsp.c @@ -380,7 +380,7 @@ int hda_dsp_resume(struct snd_sof_dev *sdev) /* turn display power on */ if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) { - ret = snd_hdac_display_power(bus, true); + ret = hda_codec_i915_get(sdev); if (ret < 0) { dev_err(bus->dev, "Cannot turn on display power on i915 after resume\n"); return ret; @@ -417,7 +417,7 @@ int hda_dsp_suspend(struct snd_sof_dev *sdev, int state) /* turn display power off */ if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) { - ret = snd_hdac_display_power(bus, false); + ret = hda_codec_i915_put(sdev); if (ret < 0) { dev_err(bus->dev, "Cannot turn OFF display power on i915 during suspend\n"); return ret; diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index d4ffdb2a76629d..1c2f8c08dc2555 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -459,8 +459,11 @@ static int hda_init_caps(struct snd_sof_dev *sdev) ret = hda_dsp_ctrl_init_chip(sdev, true); if (ret < 0) { dev_err(bus->dev, "Init chip failed with ret: %d\n", ret); - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) - snd_hdac_display_power(bus, false); + if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) { + ret = hda_codec_i915_put(sdev); + if (ret < 0) + return ret; + } return ret; } @@ -477,11 +480,9 @@ static int hda_init_caps(struct snd_sof_dev *sdev) hda_codec_probe_bus(sdev); if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) { - ret = snd_hdac_display_power(bus, false); - if (ret < 0) { - dev_err(bus->dev, "Cannot turn off display power on i915\n"); + ret = hda_codec_i915_put(sdev); + if (ret < 0) return ret; - } } /* diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index f31c1b02eb394e..875af5e287dfc9 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -512,9 +512,15 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev, * HDA Codec operations. */ int hda_codec_probe_bus(struct snd_sof_dev *sdev); + +#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI) +int hda_codec_i915_get(struct snd_sof_dev *sdev); +int hda_codec_i915_put(struct snd_sof_dev *sdev); int hda_codec_i915_init(struct snd_sof_dev *sdev); -#endif +int hda_codec_i915_exit(struct snd_sof_dev *sdev); +#endif /* CONFIG_SND_SOC_HDAC_HDMI */ +#endif /* CONFIG_SND_SOC_SOF_HDA */ /* * Trace Control. */ From d8f615a0b8dc6d9477cbb70b334f277f4b99bc20 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Mon, 12 Nov 2018 17:26:56 -0600 Subject: [PATCH 10/10] ASoC: SOF: Intel: hda: add i915_exit Start dealing with resource free to enable module load/unload FIXME: is this necessary or is this handled by the _suspend cases? Signed-off-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 1c2f8c08dc2555..a951de0edb4daf 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -767,6 +767,9 @@ int hda_dsp_remove(struct snd_sof_dev *sdev) iounmap(sdev->bar[HDA_DSP_BAR]); iounmap(bus->remap_addr); + if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) + hda_codec_i915_exit(sdev); + return 0; }