| Message ID | 20210114094615.58191-1-stephan@gerhold.net |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] ASoC: qcom: lpass: Fix hardcoded SC7810 DAI IDs | expand |
Hi Stephen, Many thanks for looking into this issue! On 14/01/2021 09:46, Stephan Gerhold wrote: > LPASS is broken on MSM8916/DragonBoard 410c since Linux 5.10. > > Attempting to play HDMI audio results in: > > ADV7533: lpass_platform_pcmops_hw_params: invalid interface: 3 > ADV7533: lpass_platform_pcmops_trigger: invalid 3 interface > apq8016-lpass-cpu 7708000.audio-controller: ASoC: error at soc_component_trigger on 7708000.audio-controller: -22 > > Attempting to record analog audio results in: > > Unable to handle kernel NULL pointer dereference at virtual address 00000000000001e4 > Internal error: Oops: 96000004 [#1] PREEMPT SMP > CPU: 1 PID: 1568 Comm: arecord Not tainted 5.11.0-rc3 #20 > Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) > pc : regmap_write+0x14/0x80 > lr : lpass_platform_pcmops_open+0xd8/0x210 [snd_soc_lpass_platform] > Call trace: > regmap_write+0x14/0x80 > lpass_platform_pcmops_open+0xd8/0x210 [snd_soc_lpass_platform] > snd_soc_component_open+0x2c/0x94 > ... > > The problem was introduced in commit 7cb37b7bd0d3 ("ASoC: qcom: Add support > for lpass hdmi driver"). The mistake made there is that lpass.h now contains > > #include <dt-bindings/sound/sc7180-lpass.h> > This thing was obviously missed in the review and testing, and its really bad idea to have multiple header files based on different SOC for the same driver. We are planning to add some basic tests in ciloop to catch such issues! IMO, Its better to sort it out now, before this gets complicated! Am thinking of making a common header file ("lpass,h") and include that in the existing SoC specific header for compatibility reasons only. In future we should just keep adding new DAI index in incremental fashion to common header file rather than creating SoC specific one! > and then the SC7810 DAI IDs are hardcoded all over lpass-cpu.c and > lpass-platform.c. But apq8016 and ipq806x have completely different > DAI IDs so now MI2S_QUATERNARY (HDMI) is invalid and > MI2S_TERTIARY (= LPASS_DP_RX in sc7180-lpass.h) is treated as HDMI port. > > Therefore, LPASS is broken on all platforms except SC7810. > > This commit fixes the problem by removing the include from lpass.h. > To check if a DAI is an HDMI port, we compare the dai_id with a new > "hdmi_dai" variable in lpass_variant. For SC7180 this is set to > LPASS_DP_RX (as before), for all other platform to some unrealistic > value (UINT_MAX). > > Cc: Srinivasa Rao Mandadapu <srivasam@codeaurora.org> > Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Fixes: 7cb37b7bd0d3 ("ASoC: qcom: Add support for lpass hdmi driver") > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > --- > Srinivas mentioned a potential different fix here: > https://lore.kernel.org/alsa-devel/4b34bd4f-e7bc-84f9-5e8a-b2348c17b7aa@linaro.org/ > > Instead of this patch, we could change the dt-bindings for LPASS, > so that all SoCs use consistent DAI IDs. TBH, Am inclined to do the right thing and make DAI ID's consistent! Like we do at the dsp afe interfaces. This will also bring in the need to add .of_xlate_dai_name callback along with fixing sc7180_lpass_cpu_dai_driver array index. Without this the code will end up very confusing! --srini > > In general, I think this might be cleaner, especially in case more special > DAIs are added in the future. However, when I made this patch (before Srinivas > mentioned it) I tried to avoid changing the dt-bindings because: > > - Changing dt-bindings after they are added is generally discouraged. > > but more importantly: > > - lpass-ipq806x.c does not seem to have PRIMARY, SECONDARY, ... > but something completely different. I know nothing about that > platform so I don't know how to handle it. > --- > sound/soc/qcom/lpass-apq8016.c | 1 + > sound/soc/qcom/lpass-cpu.c | 9 ++-- > sound/soc/qcom/lpass-ipq806x.c | 1 + > sound/soc/qcom/lpass-lpaif-reg.h | 2 +- > sound/soc/qcom/lpass-platform.c | 77 +++++++++++--------------------- > sound/soc/qcom/lpass-sc7180.c | 1 + > sound/soc/qcom/lpass.h | 4 +- > 7 files changed, 39 insertions(+), 56 deletions(-) > > diff --git a/sound/soc/qcom/lpass-apq8016.c b/sound/soc/qcom/lpass-apq8016.c > index 8507ef8f6679..40e9c66a74a9 100644 > --- a/sound/soc/qcom/lpass-apq8016.c > +++ b/sound/soc/qcom/lpass-apq8016.c > @@ -273,6 +273,7 @@ static struct lpass_variant apq8016_data = { > .num_clks = 2, > .dai_driver = apq8016_lpass_cpu_dai_driver, > .num_dai = ARRAY_SIZE(apq8016_lpass_cpu_dai_driver), > + .hdmi_dai = LPASS_HDMI_DAI_UNSUPPORTED, > .dai_osr_clk_names = (const char *[]) { > "mi2s-osr-clk0", > "mi2s-osr-clk1", > diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c > index c5e99c2d89c7..b22992050646 100644 > --- a/sound/soc/qcom/lpass-cpu.c > +++ b/sound/soc/qcom/lpass-cpu.c > @@ -707,7 +707,8 @@ static unsigned int of_lpass_cpu_parse_sd_lines(struct device *dev, > } > > static void of_lpass_cpu_parse_dai_data(struct device *dev, > - struct lpass_data *data) > + struct lpass_data *data, > + struct lpass_variant *variant) > { > struct device_node *node; > int ret, id; > @@ -724,7 +725,7 @@ static void of_lpass_cpu_parse_dai_data(struct device *dev, > dev_err(dev, "valid dai id not found: %d\n", ret); > continue; > } > - if (id == LPASS_DP_RX) { > + if (id == variant->hdmi_dai) { > data->hdmi_port_enable = 1; > dev_err(dev, "HDMI Port is enabled: %d\n", id); > } else { > @@ -766,7 +767,7 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev) > drvdata->variant = (struct lpass_variant *)match->data; > variant = drvdata->variant; > > - of_lpass_cpu_parse_dai_data(dev, drvdata); > + of_lpass_cpu_parse_dai_data(dev, drvdata, variant); > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpass-lpaif"); > > @@ -820,7 +821,7 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev) > > for (i = 0; i < variant->num_dai; i++) { > dai_id = variant->dai_driver[i].id; > - if (dai_id == LPASS_DP_RX) > + if (dai_id == variant->hdmi_dai) > continue; > > drvdata->mi2s_osr_clk[dai_id] = devm_clk_get(dev, > diff --git a/sound/soc/qcom/lpass-ipq806x.c b/sound/soc/qcom/lpass-ipq806x.c > index 92f98b4df47f..a0c7c5eb03f1 100644 > --- a/sound/soc/qcom/lpass-ipq806x.c > +++ b/sound/soc/qcom/lpass-ipq806x.c > @@ -149,6 +149,7 @@ static struct lpass_variant ipq806x_data = { > > .dai_driver = &ipq806x_lpass_cpu_dai_driver, > .num_dai = 1, > + .hdmi_dai = LPASS_HDMI_DAI_UNSUPPORTED, > .dai_osr_clk_names = (const char *[]) { > "mi2s-osr-clk", > }, > diff --git a/sound/soc/qcom/lpass-lpaif-reg.h b/sound/soc/qcom/lpass-lpaif-reg.h > index 405542832e99..332f1b9ba674 100644 > --- a/sound/soc/qcom/lpass-lpaif-reg.h > +++ b/sound/soc/qcom/lpass-lpaif-reg.h > @@ -133,7 +133,7 @@ > #define LPAIF_WRDMAPERCNT_REG(v, chan) LPAIF_WRDMA_REG_ADDR(v, 0x14, (chan)) > > #define LPAIF_INTFDMA_REG(v, chan, reg, dai_id) \ > - ((v->dai_driver[dai_id].id == LPASS_DP_RX) ? \ > + ((v->dai_driver[dai_id].id == v->hdmi_dai) ? \ > LPAIF_HDMI_RDMA##reg##_REG(v, chan) : \ > LPAIF_RDMA##reg##_REG(v, chan)) > > diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c > index d1c248590f3a..140609fe835d 100644 > --- a/sound/soc/qcom/lpass-platform.c > +++ b/sound/soc/qcom/lpass-platform.c > @@ -128,7 +128,7 @@ static int lpass_platform_pcmops_open(struct snd_soc_component *component, > return dma_ch; > } > > - if (cpu_dai->driver->id == LPASS_DP_RX) { > + if (cpu_dai->driver->id == v->hdmi_dai) { > map = drvdata->hdmiif_map; > drvdata->hdmi_substream[dma_ch] = substream; > } else { > @@ -173,7 +173,7 @@ static int lpass_platform_pcmops_close(struct snd_soc_component *component, > unsigned int dai_id = cpu_dai->driver->id; > > data = runtime->private_data; > - if (dai_id == LPASS_DP_RX) > + if (dai_id == v->hdmi_dai) > drvdata->hdmi_substream[data->dma_ch] = NULL; > else > drvdata->substream[data->dma_ch] = NULL; > @@ -205,7 +205,7 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, > > if (dir == SNDRV_PCM_STREAM_PLAYBACK) { > id = pcm_data->dma_ch; > - if (dai_id == LPASS_DP_RX) > + if (dai_id == v->hdmi_dai) > dmactl = drvdata->hdmi_rd_dmactl; > else > dmactl = drvdata->rd_dmactl; > @@ -234,8 +234,7 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, > return ret; > } > > - switch (dai_id) { > - case LPASS_DP_RX: > + if (dai_id == v->hdmi_dai) { > ret = regmap_fields_write(dmactl->burst8, id, > LPAIF_DMACTL_BURSTEN_INCR4); > if (ret) { > @@ -254,9 +253,7 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, > dev_err(soc_runtime->dev, "error updating dynbursten field: %d\n", ret); > return ret; > } > - break; > - case MI2S_PRIMARY: > - case MI2S_SECONDARY: > + } else { > ret = regmap_fields_write(dmactl->intf, id, > LPAIF_DMACTL_AUDINTF(dma_port)); > if (ret) { > @@ -264,12 +261,8 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, > ret); > return ret; > } > - > - break; > - default: > - dev_err(soc_runtime->dev, "%s: invalid interface: %d\n", __func__, dai_id); > - break; > } > + > switch (bitwidth) { > case 16: > switch (channels) { > @@ -299,22 +292,22 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, > regval = LPAIF_DMACTL_WPSCNT_ONE; > break; > case 2: > - regval = (dai_id == LPASS_DP_RX ? > + regval = (dai_id == v->hdmi_dai ? > LPAIF_DMACTL_WPSCNT_ONE : > LPAIF_DMACTL_WPSCNT_TWO); > break; > case 4: > - regval = (dai_id == LPASS_DP_RX ? > + regval = (dai_id == v->hdmi_dai ? > LPAIF_DMACTL_WPSCNT_TWO : > LPAIF_DMACTL_WPSCNT_FOUR); > break; > case 6: > - regval = (dai_id == LPASS_DP_RX ? > + regval = (dai_id == v->hdmi_dai ? > LPAIF_DMACTL_WPSCNT_THREE : > LPAIF_DMACTL_WPSCNT_SIX); > break; > case 8: > - regval = (dai_id == LPASS_DP_RX ? > + regval = (dai_id == v->hdmi_dai ? > LPAIF_DMACTL_WPSCNT_FOUR : > LPAIF_DMACTL_WPSCNT_EIGHT); > break; > @@ -354,7 +347,7 @@ static int lpass_platform_pcmops_hw_free(struct snd_soc_component *component, > struct regmap *map; > unsigned int dai_id = cpu_dai->driver->id; > > - if (dai_id == LPASS_DP_RX) > + if (dai_id == v->hdmi_dai) > map = drvdata->hdmiif_map; > else > map = drvdata->lpaif_map; > @@ -386,7 +379,7 @@ static int lpass_platform_pcmops_prepare(struct snd_soc_component *component, > > ch = pcm_data->dma_ch; > if (dir == SNDRV_PCM_STREAM_PLAYBACK) { > - if (dai_id == LPASS_DP_RX) { > + if (dai_id == v->hdmi_dai) { > dmactl = drvdata->hdmi_rd_dmactl; > map = drvdata->hdmiif_map; > } else { > @@ -456,7 +449,7 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, > ch = pcm_data->dma_ch; > if (dir == SNDRV_PCM_STREAM_PLAYBACK) { > id = pcm_data->dma_ch; > - if (dai_id == LPASS_DP_RX) { > + if (dai_id == v->hdmi_dai) { > dmactl = drvdata->hdmi_rd_dmactl; > map = drvdata->hdmiif_map; > } else { > @@ -480,8 +473,8 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, > "error writing to rdmactl reg: %d\n", ret); > return ret; > } > - switch (dai_id) { > - case LPASS_DP_RX: > + > + if (dai_id == v->hdmi_dai) { > ret = regmap_fields_write(dmactl->dyncclk, id, > LPAIF_DMACTL_DYNCLK_ON); > if (ret) { > @@ -504,9 +497,7 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, > LPAIF_IRQ_HDMI_REQ_ON_PRELOAD(ch) | > LPAIF_IRQ_HDMI_METADONE | > LPAIF_IRQ_HDMI_SDEEP_AUD_DIS(ch)); > - break; > - case MI2S_PRIMARY: > - case MI2S_SECONDARY: > + } else { > reg_irqclr = LPAIF_IRQCLEAR_REG(v, LPAIF_IRQ_PORT_HOST); > val_irqclr = LPAIF_IRQ_ALL(ch); > > @@ -514,10 +505,6 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, > reg_irqen = LPAIF_IRQEN_REG(v, LPAIF_IRQ_PORT_HOST); > val_mask = LPAIF_IRQ_ALL(ch); > val_irqen = LPAIF_IRQ_ALL(ch); > - break; > - default: > - dev_err(soc_runtime->dev, "%s: invalid %d interface\n", __func__, dai_id); > - return -EINVAL; > } > > ret = regmap_write(map, reg_irqclr, val_irqclr); > @@ -541,8 +528,8 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, > "error writing to rdmactl reg: %d\n", ret); > return ret; > } > - switch (dai_id) { > - case LPASS_DP_RX: > + > + if (dai_id == v->hdmi_dai) { > ret = regmap_fields_write(dmactl->dyncclk, id, > LPAIF_DMACTL_DYNCLK_OFF); > if (ret) { > @@ -556,16 +543,10 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, > LPAIF_IRQ_HDMI_METADONE | > LPAIF_IRQ_HDMI_SDEEP_AUD_DIS(ch)); > val_irqen = 0; > - break; > - case MI2S_PRIMARY: > - case MI2S_SECONDARY: > + } else { > reg_irqen = LPAIF_IRQEN_REG(v, LPAIF_IRQ_PORT_HOST); > val_mask = LPAIF_IRQ_ALL(ch); > val_irqen = 0; > - break; > - default: > - dev_err(soc_runtime->dev, "%s: invalid %d interface\n", __func__, dai_id); > - return -EINVAL; > } > > ret = regmap_update_bits(map, reg_irqen, val_mask, val_irqen); > @@ -595,7 +576,7 @@ static snd_pcm_uframes_t lpass_platform_pcmops_pointer( > struct regmap *map; > unsigned int dai_id = cpu_dai->driver->id; > > - if (dai_id == LPASS_DP_RX) > + if (dai_id == v->hdmi_dai) > map = drvdata->hdmiif_map; > else > map = drvdata->lpaif_map; > @@ -645,24 +626,18 @@ static irqreturn_t lpass_dma_interrupt_handler( > struct regmap *map; > unsigned int dai_id = cpu_dai->driver->id; > > - switch (dai_id) { > - case LPASS_DP_RX: > + if (dai_id == v->hdmi_dai) { > map = drvdata->hdmiif_map; > reg = LPASS_HDMITX_APP_IRQCLEAR_REG(v); > val = (LPAIF_IRQ_HDMI_REQ_ON_PRELOAD(chan) | > LPAIF_IRQ_HDMI_METADONE | > LPAIF_IRQ_HDMI_SDEEP_AUD_DIS(chan)); > - break; > - case MI2S_PRIMARY: > - case MI2S_SECONDARY: > + } else { > map = drvdata->lpaif_map; > reg = LPAIF_IRQCLEAR_REG(v, LPAIF_IRQ_PORT_HOST); > val = 0; > - break; > - default: > - dev_err(soc_runtime->dev, "%s: invalid %d interface\n", __func__, dai_id); > - return -EINVAL; > } > + > if (interrupts & LPAIF_IRQ_PER(chan)) { > > rv = regmap_write(map, reg, LPAIF_IRQ_PER(chan) | val); > @@ -826,10 +801,11 @@ static void lpass_platform_pcm_free(struct snd_soc_component *component, > static int lpass_platform_pcmops_suspend(struct snd_soc_component *component) > { > struct lpass_data *drvdata = snd_soc_component_get_drvdata(component); > + struct lpass_variant *v = drvdata->variant; > struct regmap *map; > unsigned int dai_id = component->id; > > - if (dai_id == LPASS_DP_RX) > + if (dai_id == v->hdmi_dai) > map = drvdata->hdmiif_map; > else > map = drvdata->lpaif_map; > @@ -843,10 +819,11 @@ static int lpass_platform_pcmops_suspend(struct snd_soc_component *component) > static int lpass_platform_pcmops_resume(struct snd_soc_component *component) > { > struct lpass_data *drvdata = snd_soc_component_get_drvdata(component); > + struct lpass_variant *v = drvdata->variant; > struct regmap *map; > unsigned int dai_id = component->id; > > - if (dai_id == LPASS_DP_RX) > + if (dai_id == v->hdmi_dai) > map = drvdata->hdmiif_map; > else > map = drvdata->lpaif_map; > diff --git a/sound/soc/qcom/lpass-sc7180.c b/sound/soc/qcom/lpass-sc7180.c > index 85db650c2169..76bff06543ea 100644 > --- a/sound/soc/qcom/lpass-sc7180.c > +++ b/sound/soc/qcom/lpass-sc7180.c > @@ -271,6 +271,7 @@ static struct lpass_variant sc7180_data = { > .num_clks = 3, > .dai_driver = sc7180_lpass_cpu_dai_driver, > .num_dai = ARRAY_SIZE(sc7180_lpass_cpu_dai_driver), > + .hdmi_dai = LPASS_DP_RX, > .dai_osr_clk_names = (const char *[]) { > "mclk0", > "null", > diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h > index 0195372905ed..2de0632c1fb7 100644 > --- a/sound/soc/qcom/lpass.h > +++ b/sound/soc/qcom/lpass.h > @@ -12,7 +12,6 @@ > #include <linux/compiler.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > -#include <dt-bindings/sound/sc7180-lpass.h> > #include "lpass-hdmi.h" > > #define LPASS_AHBIX_CLOCK_FREQUENCY 131072000 > @@ -20,6 +19,8 @@ > #define LPASS_MAX_DMA_CHANNELS (8) > #define LPASS_MAX_HDMI_DMA_CHANNELS (4) > > +#define LPASS_HDMI_DAI_UNSUPPORTED UINT_MAX > + > #define QCOM_REGMAP_FIELD_ALLOC(d, m, f, mf) \ > do { \ > mf = devm_regmap_field_alloc(d, m, f); \ > @@ -245,6 +246,7 @@ struct lpass_variant { > /* SOC specific dais */ > struct snd_soc_dai_driver *dai_driver; > int num_dai; > + unsigned int hdmi_dai; > const char * const *dai_osr_clk_names; > const char * const *dai_bit_clk_names; > >
Hi Srinivas, On Fri, Jan 15, 2021 at 04:14:05PM +0000, Srinivas Kandagatla wrote: > On 14/01/2021 09:46, Stephan Gerhold wrote: > > [...] > > The problem was introduced in commit 7cb37b7bd0d3 ("ASoC: qcom: Add support > > for lpass hdmi driver"). The mistake made there is that lpass.h now contains > > > > #include <dt-bindings/sound/sc7180-lpass.h> > > > > This thing was obviously missed in the review and testing, and its really > bad idea to have multiple header files based on different SOC for the same > driver. We are planning to add some basic tests in ciloop to catch such > issues! > > IMO, Its better to sort it out now, before this gets complicated! > > Am thinking of making a common header file ("lpass,h") and include that in > the existing SoC specific header for compatibility reasons only. > > In future we should just keep adding new DAI index in incremental fashion to > common header file rather than creating SoC specific one! > > > > [...] > > --- > > Srinivas mentioned a potential different fix here: > > https://lore.kernel.org/alsa-devel/4b34bd4f-e7bc-84f9-5e8a-b2348c17b7aa@linaro.org/ > > > > Instead of this patch, we could change the dt-bindings for LPASS, > > so that all SoCs use consistent DAI IDs. > > TBH, Am inclined to do the right thing and make DAI ID's consistent! > Like we do at the dsp afe interfaces. > > This will also bring in the need to add .of_xlate_dai_name callback along > with fixing sc7180_lpass_cpu_dai_driver array index. > > Without this the code will end up very confusing! > I agree that this would be cleaner, as I mentioned here: > > > > > In general, I think this might be cleaner, especially in case more special > > DAIs are added in the future. However, when I made this patch (before Srinivas > > mentioned it) I tried to avoid changing the dt-bindings because: > > > > - Changing dt-bindings after they are added is generally discouraged. > > > > but more importantly: > > > > - lpass-ipq806x.c does not seem to have PRIMARY, SECONDARY, ... > > but something completely different. I know nothing about that > > platform so I don't know how to handle it. > ... but it's still not clear to me how to handle ipq806x. The DAIs it has don't really look similar to what MSM8916 and SC7180 have. Right now it declares just a single DAI, but multiple "ports": enum lpaif_i2s_ports { IPQ806X_LPAIF_I2S_PORT_CODEC_SPK, IPQ806X_LPAIF_I2S_PORT_CODEC_MIC, IPQ806X_LPAIF_I2S_PORT_SEC_SPK, IPQ806X_LPAIF_I2S_PORT_SEC_MIC, IPQ806X_LPAIF_I2S_PORT_MI2S, }; static struct snd_soc_dai_driver ipq806x_lpass_cpu_dai_driver = { .id = IPQ806X_LPAIF_I2S_PORT_MI2S, /* ... */ }; I suppose we could just declare this as MI2S_PRIMARY but not sure if that is accurate. Do you have more information about this platform? Thanks, Stephan
On 15/01/2021 16:49, Stephan Gerhold wrote: > Right now it declares just a single DAI, but multiple "ports": > > enum lpaif_i2s_ports { > IPQ806X_LPAIF_I2S_PORT_CODEC_SPK, > IPQ806X_LPAIF_I2S_PORT_CODEC_MIC, > IPQ806X_LPAIF_I2S_PORT_SEC_SPK, > IPQ806X_LPAIF_I2S_PORT_SEC_MIC, > IPQ806X_LPAIF_I2S_PORT_MI2S, > }; > > static struct snd_soc_dai_driver ipq806x_lpass_cpu_dai_driver = { > .id = IPQ806X_LPAIF_I2S_PORT_MI2S, > /* ... */ > }; > > I suppose we could just declare this as MI2S_PRIMARY but not sure if > that is accurate. Do you have more information about this platform? Looking at the specs it does show that it has 0-4 total 5 I2S interfaces, however Am unable to find a proper name similar to other MI2S. This one is the last one (4)! --srini
On Fri, Jan 15, 2021 at 05:15:53PM +0000, Srinivas Kandagatla wrote: > On 15/01/2021 16:49, Stephan Gerhold wrote: > > Right now it declares just a single DAI, but multiple "ports": > > > > enum lpaif_i2s_ports { > > IPQ806X_LPAIF_I2S_PORT_CODEC_SPK, > > IPQ806X_LPAIF_I2S_PORT_CODEC_MIC, > > IPQ806X_LPAIF_I2S_PORT_SEC_SPK, > > IPQ806X_LPAIF_I2S_PORT_SEC_MIC, > > IPQ806X_LPAIF_I2S_PORT_MI2S, > > }; > > > > static struct snd_soc_dai_driver ipq806x_lpass_cpu_dai_driver = { > > .id = IPQ806X_LPAIF_I2S_PORT_MI2S, > > /* ... */ > > }; > > > > I suppose we could just declare this as MI2S_PRIMARY but not sure if > > that is accurate. Do you have more information about this platform? > > Looking at the specs it does show that it has 0-4 total 5 I2S interfaces, > however Am unable to find a proper name similar to other MI2S. > This one is the last one (4)! > I'm still a bit unsure how this would fit into the shared lpass.h dt-bindings, given that we need MI2S_PRIMARY, etc for MSM8916/SC7810 but some different ones for IPQ806X. But I would also need to check how to implement .of_xlate_dai_name first for example, so I think it's easier if you can prepare a patch to implement your idea. I would be happy to test it. :) Then we can just drop this patch set, I don't mind. Thanks! Stephan
diff --git a/sound/soc/qcom/lpass-apq8016.c b/sound/soc/qcom/lpass-apq8016.c index 8507ef8f6679..40e9c66a74a9 100644 --- a/sound/soc/qcom/lpass-apq8016.c +++ b/sound/soc/qcom/lpass-apq8016.c @@ -273,6 +273,7 @@ static struct lpass_variant apq8016_data = { .num_clks = 2, .dai_driver = apq8016_lpass_cpu_dai_driver, .num_dai = ARRAY_SIZE(apq8016_lpass_cpu_dai_driver), + .hdmi_dai = LPASS_HDMI_DAI_UNSUPPORTED, .dai_osr_clk_names = (const char *[]) { "mi2s-osr-clk0", "mi2s-osr-clk1", diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c index c5e99c2d89c7..b22992050646 100644 --- a/sound/soc/qcom/lpass-cpu.c +++ b/sound/soc/qcom/lpass-cpu.c @@ -707,7 +707,8 @@ static unsigned int of_lpass_cpu_parse_sd_lines(struct device *dev, } static void of_lpass_cpu_parse_dai_data(struct device *dev, - struct lpass_data *data) + struct lpass_data *data, + struct lpass_variant *variant) { struct device_node *node; int ret, id; @@ -724,7 +725,7 @@ static void of_lpass_cpu_parse_dai_data(struct device *dev, dev_err(dev, "valid dai id not found: %d\n", ret); continue; } - if (id == LPASS_DP_RX) { + if (id == variant->hdmi_dai) { data->hdmi_port_enable = 1; dev_err(dev, "HDMI Port is enabled: %d\n", id); } else { @@ -766,7 +767,7 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev) drvdata->variant = (struct lpass_variant *)match->data; variant = drvdata->variant; - of_lpass_cpu_parse_dai_data(dev, drvdata); + of_lpass_cpu_parse_dai_data(dev, drvdata, variant); res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpass-lpaif"); @@ -820,7 +821,7 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev) for (i = 0; i < variant->num_dai; i++) { dai_id = variant->dai_driver[i].id; - if (dai_id == LPASS_DP_RX) + if (dai_id == variant->hdmi_dai) continue; drvdata->mi2s_osr_clk[dai_id] = devm_clk_get(dev, diff --git a/sound/soc/qcom/lpass-ipq806x.c b/sound/soc/qcom/lpass-ipq806x.c index 92f98b4df47f..a0c7c5eb03f1 100644 --- a/sound/soc/qcom/lpass-ipq806x.c +++ b/sound/soc/qcom/lpass-ipq806x.c @@ -149,6 +149,7 @@ static struct lpass_variant ipq806x_data = { .dai_driver = &ipq806x_lpass_cpu_dai_driver, .num_dai = 1, + .hdmi_dai = LPASS_HDMI_DAI_UNSUPPORTED, .dai_osr_clk_names = (const char *[]) { "mi2s-osr-clk", }, diff --git a/sound/soc/qcom/lpass-lpaif-reg.h b/sound/soc/qcom/lpass-lpaif-reg.h index 405542832e99..332f1b9ba674 100644 --- a/sound/soc/qcom/lpass-lpaif-reg.h +++ b/sound/soc/qcom/lpass-lpaif-reg.h @@ -133,7 +133,7 @@ #define LPAIF_WRDMAPERCNT_REG(v, chan) LPAIF_WRDMA_REG_ADDR(v, 0x14, (chan)) #define LPAIF_INTFDMA_REG(v, chan, reg, dai_id) \ - ((v->dai_driver[dai_id].id == LPASS_DP_RX) ? \ + ((v->dai_driver[dai_id].id == v->hdmi_dai) ? \ LPAIF_HDMI_RDMA##reg##_REG(v, chan) : \ LPAIF_RDMA##reg##_REG(v, chan)) diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c index d1c248590f3a..140609fe835d 100644 --- a/sound/soc/qcom/lpass-platform.c +++ b/sound/soc/qcom/lpass-platform.c @@ -128,7 +128,7 @@ static int lpass_platform_pcmops_open(struct snd_soc_component *component, return dma_ch; } - if (cpu_dai->driver->id == LPASS_DP_RX) { + if (cpu_dai->driver->id == v->hdmi_dai) { map = drvdata->hdmiif_map; drvdata->hdmi_substream[dma_ch] = substream; } else { @@ -173,7 +173,7 @@ static int lpass_platform_pcmops_close(struct snd_soc_component *component, unsigned int dai_id = cpu_dai->driver->id; data = runtime->private_data; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) drvdata->hdmi_substream[data->dma_ch] = NULL; else drvdata->substream[data->dma_ch] = NULL; @@ -205,7 +205,7 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, if (dir == SNDRV_PCM_STREAM_PLAYBACK) { id = pcm_data->dma_ch; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) dmactl = drvdata->hdmi_rd_dmactl; else dmactl = drvdata->rd_dmactl; @@ -234,8 +234,7 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, return ret; } - switch (dai_id) { - case LPASS_DP_RX: + if (dai_id == v->hdmi_dai) { ret = regmap_fields_write(dmactl->burst8, id, LPAIF_DMACTL_BURSTEN_INCR4); if (ret) { @@ -254,9 +253,7 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, dev_err(soc_runtime->dev, "error updating dynbursten field: %d\n", ret); return ret; } - break; - case MI2S_PRIMARY: - case MI2S_SECONDARY: + } else { ret = regmap_fields_write(dmactl->intf, id, LPAIF_DMACTL_AUDINTF(dma_port)); if (ret) { @@ -264,12 +261,8 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, ret); return ret; } - - break; - default: - dev_err(soc_runtime->dev, "%s: invalid interface: %d\n", __func__, dai_id); - break; } + switch (bitwidth) { case 16: switch (channels) { @@ -299,22 +292,22 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, regval = LPAIF_DMACTL_WPSCNT_ONE; break; case 2: - regval = (dai_id == LPASS_DP_RX ? + regval = (dai_id == v->hdmi_dai ? LPAIF_DMACTL_WPSCNT_ONE : LPAIF_DMACTL_WPSCNT_TWO); break; case 4: - regval = (dai_id == LPASS_DP_RX ? + regval = (dai_id == v->hdmi_dai ? LPAIF_DMACTL_WPSCNT_TWO : LPAIF_DMACTL_WPSCNT_FOUR); break; case 6: - regval = (dai_id == LPASS_DP_RX ? + regval = (dai_id == v->hdmi_dai ? LPAIF_DMACTL_WPSCNT_THREE : LPAIF_DMACTL_WPSCNT_SIX); break; case 8: - regval = (dai_id == LPASS_DP_RX ? + regval = (dai_id == v->hdmi_dai ? LPAIF_DMACTL_WPSCNT_FOUR : LPAIF_DMACTL_WPSCNT_EIGHT); break; @@ -354,7 +347,7 @@ static int lpass_platform_pcmops_hw_free(struct snd_soc_component *component, struct regmap *map; unsigned int dai_id = cpu_dai->driver->id; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) map = drvdata->hdmiif_map; else map = drvdata->lpaif_map; @@ -386,7 +379,7 @@ static int lpass_platform_pcmops_prepare(struct snd_soc_component *component, ch = pcm_data->dma_ch; if (dir == SNDRV_PCM_STREAM_PLAYBACK) { - if (dai_id == LPASS_DP_RX) { + if (dai_id == v->hdmi_dai) { dmactl = drvdata->hdmi_rd_dmactl; map = drvdata->hdmiif_map; } else { @@ -456,7 +449,7 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, ch = pcm_data->dma_ch; if (dir == SNDRV_PCM_STREAM_PLAYBACK) { id = pcm_data->dma_ch; - if (dai_id == LPASS_DP_RX) { + if (dai_id == v->hdmi_dai) { dmactl = drvdata->hdmi_rd_dmactl; map = drvdata->hdmiif_map; } else { @@ -480,8 +473,8 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, "error writing to rdmactl reg: %d\n", ret); return ret; } - switch (dai_id) { - case LPASS_DP_RX: + + if (dai_id == v->hdmi_dai) { ret = regmap_fields_write(dmactl->dyncclk, id, LPAIF_DMACTL_DYNCLK_ON); if (ret) { @@ -504,9 +497,7 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, LPAIF_IRQ_HDMI_REQ_ON_PRELOAD(ch) | LPAIF_IRQ_HDMI_METADONE | LPAIF_IRQ_HDMI_SDEEP_AUD_DIS(ch)); - break; - case MI2S_PRIMARY: - case MI2S_SECONDARY: + } else { reg_irqclr = LPAIF_IRQCLEAR_REG(v, LPAIF_IRQ_PORT_HOST); val_irqclr = LPAIF_IRQ_ALL(ch); @@ -514,10 +505,6 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, reg_irqen = LPAIF_IRQEN_REG(v, LPAIF_IRQ_PORT_HOST); val_mask = LPAIF_IRQ_ALL(ch); val_irqen = LPAIF_IRQ_ALL(ch); - break; - default: - dev_err(soc_runtime->dev, "%s: invalid %d interface\n", __func__, dai_id); - return -EINVAL; } ret = regmap_write(map, reg_irqclr, val_irqclr); @@ -541,8 +528,8 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, "error writing to rdmactl reg: %d\n", ret); return ret; } - switch (dai_id) { - case LPASS_DP_RX: + + if (dai_id == v->hdmi_dai) { ret = regmap_fields_write(dmactl->dyncclk, id, LPAIF_DMACTL_DYNCLK_OFF); if (ret) { @@ -556,16 +543,10 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, LPAIF_IRQ_HDMI_METADONE | LPAIF_IRQ_HDMI_SDEEP_AUD_DIS(ch)); val_irqen = 0; - break; - case MI2S_PRIMARY: - case MI2S_SECONDARY: + } else { reg_irqen = LPAIF_IRQEN_REG(v, LPAIF_IRQ_PORT_HOST); val_mask = LPAIF_IRQ_ALL(ch); val_irqen = 0; - break; - default: - dev_err(soc_runtime->dev, "%s: invalid %d interface\n", __func__, dai_id); - return -EINVAL; } ret = regmap_update_bits(map, reg_irqen, val_mask, val_irqen); @@ -595,7 +576,7 @@ static snd_pcm_uframes_t lpass_platform_pcmops_pointer( struct regmap *map; unsigned int dai_id = cpu_dai->driver->id; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) map = drvdata->hdmiif_map; else map = drvdata->lpaif_map; @@ -645,24 +626,18 @@ static irqreturn_t lpass_dma_interrupt_handler( struct regmap *map; unsigned int dai_id = cpu_dai->driver->id; - switch (dai_id) { - case LPASS_DP_RX: + if (dai_id == v->hdmi_dai) { map = drvdata->hdmiif_map; reg = LPASS_HDMITX_APP_IRQCLEAR_REG(v); val = (LPAIF_IRQ_HDMI_REQ_ON_PRELOAD(chan) | LPAIF_IRQ_HDMI_METADONE | LPAIF_IRQ_HDMI_SDEEP_AUD_DIS(chan)); - break; - case MI2S_PRIMARY: - case MI2S_SECONDARY: + } else { map = drvdata->lpaif_map; reg = LPAIF_IRQCLEAR_REG(v, LPAIF_IRQ_PORT_HOST); val = 0; - break; - default: - dev_err(soc_runtime->dev, "%s: invalid %d interface\n", __func__, dai_id); - return -EINVAL; } + if (interrupts & LPAIF_IRQ_PER(chan)) { rv = regmap_write(map, reg, LPAIF_IRQ_PER(chan) | val); @@ -826,10 +801,11 @@ static void lpass_platform_pcm_free(struct snd_soc_component *component, static int lpass_platform_pcmops_suspend(struct snd_soc_component *component) { struct lpass_data *drvdata = snd_soc_component_get_drvdata(component); + struct lpass_variant *v = drvdata->variant; struct regmap *map; unsigned int dai_id = component->id; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) map = drvdata->hdmiif_map; else map = drvdata->lpaif_map; @@ -843,10 +819,11 @@ static int lpass_platform_pcmops_suspend(struct snd_soc_component *component) static int lpass_platform_pcmops_resume(struct snd_soc_component *component) { struct lpass_data *drvdata = snd_soc_component_get_drvdata(component); + struct lpass_variant *v = drvdata->variant; struct regmap *map; unsigned int dai_id = component->id; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) map = drvdata->hdmiif_map; else map = drvdata->lpaif_map; diff --git a/sound/soc/qcom/lpass-sc7180.c b/sound/soc/qcom/lpass-sc7180.c index 85db650c2169..76bff06543ea 100644 --- a/sound/soc/qcom/lpass-sc7180.c +++ b/sound/soc/qcom/lpass-sc7180.c @@ -271,6 +271,7 @@ static struct lpass_variant sc7180_data = { .num_clks = 3, .dai_driver = sc7180_lpass_cpu_dai_driver, .num_dai = ARRAY_SIZE(sc7180_lpass_cpu_dai_driver), + .hdmi_dai = LPASS_DP_RX, .dai_osr_clk_names = (const char *[]) { "mclk0", "null", diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h index 0195372905ed..2de0632c1fb7 100644 --- a/sound/soc/qcom/lpass.h +++ b/sound/soc/qcom/lpass.h @@ -12,7 +12,6 @@ #include <linux/compiler.h> #include <linux/platform_device.h> #include <linux/regmap.h> -#include <dt-bindings/sound/sc7180-lpass.h> #include "lpass-hdmi.h" #define LPASS_AHBIX_CLOCK_FREQUENCY 131072000 @@ -20,6 +19,8 @@ #define LPASS_MAX_DMA_CHANNELS (8) #define LPASS_MAX_HDMI_DMA_CHANNELS (4) +#define LPASS_HDMI_DAI_UNSUPPORTED UINT_MAX + #define QCOM_REGMAP_FIELD_ALLOC(d, m, f, mf) \ do { \ mf = devm_regmap_field_alloc(d, m, f); \ @@ -245,6 +246,7 @@ struct lpass_variant { /* SOC specific dais */ struct snd_soc_dai_driver *dai_driver; int num_dai; + unsigned int hdmi_dai; const char * const *dai_osr_clk_names; const char * const *dai_bit_clk_names;
LPASS is broken on MSM8916/DragonBoard 410c since Linux 5.10. Attempting to play HDMI audio results in: ADV7533: lpass_platform_pcmops_hw_params: invalid interface: 3 ADV7533: lpass_platform_pcmops_trigger: invalid 3 interface apq8016-lpass-cpu 7708000.audio-controller: ASoC: error at soc_component_trigger on 7708000.audio-controller: -22 Attempting to record analog audio results in: Unable to handle kernel NULL pointer dereference at virtual address 00000000000001e4 Internal error: Oops: 96000004 [#1] PREEMPT SMP CPU: 1 PID: 1568 Comm: arecord Not tainted 5.11.0-rc3 #20 Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) pc : regmap_write+0x14/0x80 lr : lpass_platform_pcmops_open+0xd8/0x210 [snd_soc_lpass_platform] Call trace: regmap_write+0x14/0x80 lpass_platform_pcmops_open+0xd8/0x210 [snd_soc_lpass_platform] snd_soc_component_open+0x2c/0x94 ... The problem was introduced in commit 7cb37b7bd0d3 ("ASoC: qcom: Add support for lpass hdmi driver"). The mistake made there is that lpass.h now contains #include <dt-bindings/sound/sc7180-lpass.h> and then the SC7810 DAI IDs are hardcoded all over lpass-cpu.c and lpass-platform.c. But apq8016 and ipq806x have completely different DAI IDs so now MI2S_QUATERNARY (HDMI) is invalid and MI2S_TERTIARY (= LPASS_DP_RX in sc7180-lpass.h) is treated as HDMI port. Therefore, LPASS is broken on all platforms except SC7810. This commit fixes the problem by removing the include from lpass.h. To check if a DAI is an HDMI port, we compare the dai_id with a new "hdmi_dai" variable in lpass_variant. For SC7180 this is set to LPASS_DP_RX (as before), for all other platform to some unrealistic value (UINT_MAX). Cc: Srinivasa Rao Mandadapu <srivasam@codeaurora.org> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Fixes: 7cb37b7bd0d3 ("ASoC: qcom: Add support for lpass hdmi driver") Signed-off-by: Stephan Gerhold <stephan@gerhold.net> --- Srinivas mentioned a potential different fix here: https://lore.kernel.org/alsa-devel/4b34bd4f-e7bc-84f9-5e8a-b2348c17b7aa@linaro.org/ Instead of this patch, we could change the dt-bindings for LPASS, so that all SoCs use consistent DAI IDs. In general, I think this might be cleaner, especially in case more special DAIs are added in the future. However, when I made this patch (before Srinivas mentioned it) I tried to avoid changing the dt-bindings because: - Changing dt-bindings after they are added is generally discouraged. but more importantly: - lpass-ipq806x.c does not seem to have PRIMARY, SECONDARY, ... but something completely different. I know nothing about that platform so I don't know how to handle it. --- sound/soc/qcom/lpass-apq8016.c | 1 + sound/soc/qcom/lpass-cpu.c | 9 ++-- sound/soc/qcom/lpass-ipq806x.c | 1 + sound/soc/qcom/lpass-lpaif-reg.h | 2 +- sound/soc/qcom/lpass-platform.c | 77 +++++++++++--------------------- sound/soc/qcom/lpass-sc7180.c | 1 + sound/soc/qcom/lpass.h | 4 +- 7 files changed, 39 insertions(+), 56 deletions(-)