| Message ID | 20211202145505.58852-6-stephan@gerhold.net |
|---|---|
| State | Accepted |
| Commit | a78a42fb48b8f261ab122c929f78c272ffc26d1b |
| Headers | show |
| Series | ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6 | expand |
Hi Srinivas, Thanks for your review! On Fri, Dec 03, 2021 at 10:35:08AM +0000, Srinivas Kandagatla wrote: > I have tested DB410c this use case in the past using similar patch [1]. > Did you use a different modem DSP firmware? (An older one maybe?) In my tests the newer ones seem to have QDSP6 audio completely broken, my DB410c simply rebooted when I tried it. > On 02/12/2021 14:55, Stephan Gerhold wrote: > > The apq8016-sbc-sndcard is designed to be used with the LPASS drivers > > (bypassing the combined audio/modem DSP in MSM8916/APQ8016). > > Make it possible to use QDSP6 audio instead for the msm8916-qdsp6-sndcard. > > > > This only requires adding some additional hooks that set up the DPCM > > backends correctly. Similar code is already used in drivers for newer > > SoCs such as apq8096.c, sdm845.c and sm8250.c. > > > > A slightly different initialization sequence is used for the apq8016-sbc > > and msm8916-qdsp6 sound card by defining the apq8016_sbc_add_ops() > > function as device match data. > > Other alternative is to reuse card->name populated from "qcom,model" > property to differentiate between both of these. > > This should also help in differentiating UCM configs. > I have "qdsp6" in card->components to differentiate the setups in UCM configs. I think this is a more flexible approach than adding it to the card model. It can be checked in UCM using ${CardComponents}. In my setup the card "model" identifies the device in use (e.g. smartphone X with a stereo speaker setup). This device might use the DSP bypass (apq8016-sbc-sndcard) or QDSP6 (msm8916-qdsp6-sndcard), depending on user preference. In UCM this is detected by checking if ${CardComponents} contains "qdsp6" or not. The reason for supporting both setups is that they both have their advantages and disadvantages. The DSP must be used when the modem is needed, but otherwise the LPASS driver tends to give more easy control about sample rate, latency etc. > > > > > Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > Few minor nits, other than that it LGTM, > > Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > > --- > > sound/soc/qcom/apq8016_sbc.c | 134 +++++++++++++++++++++++++++++++++-- > > 1 file changed, 129 insertions(+), 5 deletions(-) > > > > diff --git a/sound/soc/qcom/apq8016_sbc.c b/sound/soc/qcom/apq8016_sbc.c > > index ba2a98268ee4..f9d69375320e 100644 > > --- a/sound/soc/qcom/apq8016_sbc.c > > +++ b/sound/soc/qcom/apq8016_sbc.c > > [...] > > +static int msm8916_qdsp6_startup(struct snd_pcm_substream *substream) > > +{ > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > + struct snd_soc_card *card = rtd->card; > > + struct apq8016_sbc_data *data = snd_soc_card_get_drvdata(card); > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > + int mi2s, ret; > > + > > + mi2s = qdsp6_dai_get_lpass_id(cpu_dai); > > + if (mi2s < 0) > > + return mi2s; > > + > > + if (++data->mi2s_clk_count[mi2s] > 1) > > + return 0; > > + > > Am assuming that as you are not setting any DIGITAL CDC clock here you might > be using an external codec. > For apq8016-sbc the digital clock is controlled by msm8916-wcd-digital through the reference in the device tree (<&gcc GCC_CODEC_DIGCODEC_CLK>). It must be carefully managed, because it is needed for register access in that driver. Since QDSP6 also allows controlling this clock though LPAIF_DIG_CLK it is a bit of a hack to bypass it using the GCC driver. However, I kept the same setup for simplicity and it has been working just fine so far. AFAICT in your commit you simply turn on the clock twice, once directly using GCC and once indirectly via LPAIF_DIG_CLK in QDSP6. :-) > > + ret = snd_soc_dai_set_sysclk(cpu_dai, LPAIF_BIT_CLK, MI2S_BCLK_RATE, 0); > > + if (ret) > > + dev_err(card->dev, "Failed to enable LPAIF bit clk: %d\n", ret); > > + return ret; > > +} > > + > > [...] > > @@ -148,11 +266,16 @@ static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = { > > static int apq8016_sbc_platform_probe(struct platform_device *pdev) > > { > > + void (*add_ops)(struct snd_soc_card *card); > > struct device *dev = &pdev->dev; > > struct snd_soc_card *card; > > struct apq8016_sbc_data *data; > > int ret; > > + add_ops = device_get_match_data(&pdev->dev); > > + if (!add_ops) > > + return -EINVAL; > > We will never hit the error case here because without a match we can not > even enter the probe function. > Theoretically it's possible to create platform devices through other ways than the device tree (think of old board C files for example). I agree that nobody should do that, but having this check here at least avoids a NULL pointer dereference in this unlikely scenario. Please let me know if I should remove it anyway, that's fine for me! Thanks, Stephan
On 03/12/2021 14:36, Stephan Gerhold wrote: > Hi Srinivas, > > Thanks for your review! > > On Fri, Dec 03, 2021 at 10:35:08AM +0000, Srinivas Kandagatla wrote: >> I have tested DB410c this use case in the past using similar patch [1]. >> > > Did you use a different modem DSP firmware? (An older one maybe?) It was very old which came with some Android release I guess. > In my tests the newer ones seem to have QDSP6 audio completely broken, > my DB410c simply rebooted when I tried it. > >> On 02/12/2021 14:55, Stephan Gerhold wrote: >>> The apq8016-sbc-sndcard is designed to be used with the LPASS drivers >>> (bypassing the combined audio/modem DSP in MSM8916/APQ8016). >>> Make it possible to use QDSP6 audio instead for the msm8916-qdsp6-sndcard. >>> >>> This only requires adding some additional hooks that set up the DPCM >>> backends correctly. Similar code is already used in drivers for newer >>> SoCs such as apq8096.c, sdm845.c and sm8250.c. >>> >>> A slightly different initialization sequence is used for the apq8016-sbc >>> and msm8916-qdsp6 sound card by defining the apq8016_sbc_add_ops() >>> function as device match data. >> >> Other alternative is to reuse card->name populated from "qcom,model" >> property to differentiate between both of these. >> >> This should also help in differentiating UCM configs. >> > > I have "qdsp6" in card->components to differentiate the setups in UCM > configs. I think this is a more flexible approach than adding it to the > card model. It can be checked in UCM using ${CardComponents}. > > In my setup the card "model" identifies the device in use (e.g. > smartphone X with a stereo speaker setup). This device might use the > DSP bypass (apq8016-sbc-sndcard) or QDSP6 (msm8916-qdsp6-sndcard), > depending on user preference. In UCM this is detected by checking > if ${CardComponents} contains "qdsp6" or not. That is other way to do it too. > > The reason for supporting both setups is that they both have their > advantages and disadvantages. The DSP must be used when the modem is > needed, but otherwise the LPASS driver tends to give more easy control > about sample rate, latency etc. > >> >>> >>> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net> >> >> Few minor nits, other than that it LGTM, >> >> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >>> --- >>> sound/soc/qcom/apq8016_sbc.c | 134 +++++++++++++++++++++++++++++++++-- >>> 1 file changed, 129 insertions(+), 5 deletions(-) >>> >>> diff --git a/sound/soc/qcom/apq8016_sbc.c b/sound/soc/qcom/apq8016_sbc.c >>> index ba2a98268ee4..f9d69375320e 100644 >>> --- a/sound/soc/qcom/apq8016_sbc.c >>> +++ b/sound/soc/qcom/apq8016_sbc.c >>> [...] >>> +static int msm8916_qdsp6_startup(struct snd_pcm_substream *substream) >>> +{ >>> + struct snd_soc_pcm_runtime *rtd = substream->private_data; >>> + struct snd_soc_card *card = rtd->card; >>> + struct apq8016_sbc_data *data = snd_soc_card_get_drvdata(card); >>> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); >>> + int mi2s, ret; >>> + >>> + mi2s = qdsp6_dai_get_lpass_id(cpu_dai); >>> + if (mi2s < 0) >>> + return mi2s; >>> + >>> + if (++data->mi2s_clk_count[mi2s] > 1) >>> + return 0; >>> + >> >> Am assuming that as you are not setting any DIGITAL CDC clock here you might >> be using an external codec. >> > > For apq8016-sbc the digital clock is controlled by msm8916-wcd-digital > through the reference in the device tree (<&gcc GCC_CODEC_DIGCODEC_CLK>). > It must be carefully managed, because it is needed for register access > in that driver. > > Since QDSP6 also allows controlling this clock though LPAIF_DIG_CLK > it is a bit of a hack to bypass it using the GCC driver. However, I kept > the same setup for simplicity and it has been working just fine so far. Nice! > > AFAICT in your commit you simply turn on the clock twice, once directly > using GCC and once indirectly via LPAIF_DIG_CLK in QDSP6. :-) Yes, that was bit of test code TBH. > >>> + ret = snd_soc_dai_set_sysclk(cpu_dai, LPAIF_BIT_CLK, MI2S_BCLK_RATE, 0); >>> + if (ret) >>> + dev_err(card->dev, "Failed to enable LPAIF bit clk: %d\n", ret); >>> + return ret; >>> +} >>> + >>> [...] >>> @@ -148,11 +266,16 @@ static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = { >>> static int apq8016_sbc_platform_probe(struct platform_device *pdev) >>> { >>> + void (*add_ops)(struct snd_soc_card *card); >>> struct device *dev = &pdev->dev; >>> struct snd_soc_card *card; >>> struct apq8016_sbc_data *data; >>> int ret; >>> + add_ops = device_get_match_data(&pdev->dev); >>> + if (!add_ops) >>> + return -EINVAL; >> >> We will never hit the error case here because without a match we can not >> even enter the probe function. >> > > Theoretically it's possible to create platform devices through other > ways than the device tree (think of old board C files for example). > I agree that nobody should do that, but having this check here > at least avoids a NULL pointer dereference in this unlikely scenario. > > Please let me know if I should remove it anyway, that's fine for me! TBH, I don't have very strong opinion on this. --srini > > Thanks, > Stephan >
On Fri, Dec 03, 2021 at 02:52:43PM +0000, Srinivas Kandagatla wrote: > On 03/12/2021 14:36, Stephan Gerhold wrote: > > On Fri, Dec 03, 2021 at 10:35:08AM +0000, Srinivas Kandagatla wrote: > > > I have tested DB410c this use case in the past using similar patch [1]. > > > > > > > Did you use a different modem DSP firmware? (An older one maybe?) > > It was very old which came with some Android release I guess. > Right, that should be similar to the ones used on MSM8916 smartphones/tablets. I was really glad that the qdsp6 drivers (q6asm, q6afe, ...) worked without any changes on MSM8916 by the way, thanks a lot for all your work on them! > > > > @@ -148,11 +266,16 @@ static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = { > > > > static int apq8016_sbc_platform_probe(struct platform_device *pdev) > > > > { > > > > + void (*add_ops)(struct snd_soc_card *card); > > > > struct device *dev = &pdev->dev; > > > > struct snd_soc_card *card; > > > > struct apq8016_sbc_data *data; > > > > int ret; > > > > + add_ops = device_get_match_data(&pdev->dev); > > > > + if (!add_ops) > > > > + return -EINVAL; > > > > > > We will never hit the error case here because without a match we can not > > > even enter the probe function. > > > > > > > Theoretically it's possible to create platform devices through other > > ways than the device tree (think of old board C files for example). > > I agree that nobody should do that, but having this check here > > at least avoids a NULL pointer dereference in this unlikely scenario. > > > > Please let me know if I should remove it anyway, that's fine for me! > > TBH, I don't have very strong opinion on this. > Great, can I assume your Reviewed-by: applies without any changes then? Thanks, Stephan
On 03/12/2021 15:09, Stephan Gerhold wrote: >>>>> + add_ops = device_get_match_data(&pdev->dev); >>>>> + if (!add_ops) >>>>> + return -EINVAL; >>>> We will never hit the error case here because without a match we can not >>>> even enter the probe function. >>>> >>> Theoretically it's possible to create platform devices through other >>> ways than the device tree (think of old board C files for example). >>> I agree that nobody should do that, but having this check here >>> at least avoids a NULL pointer dereference in this unlikely scenario. >>> >>> Please let me know if I should remove it anyway, that's fine for me! >> TBH, I don't have very strong opinion on this. >> > Great, can I assume your Reviewed-by: applies without any changes then? yes sure. --srini
diff --git a/sound/soc/qcom/apq8016_sbc.c b/sound/soc/qcom/apq8016_sbc.c index ba2a98268ee4..f9d69375320e 100644 --- a/sound/soc/qcom/apq8016_sbc.c +++ b/sound/soc/qcom/apq8016_sbc.c @@ -17,6 +17,9 @@ #include <uapi/linux/input-event-codes.h> #include <dt-bindings/sound/apq8016-lpass.h> #include "common.h" +#include "qdsp6/q6afe.h" + +#define MI2S_COUNT (MI2S_QUATERNARY + 1) struct apq8016_sbc_data { struct snd_soc_card card; @@ -24,6 +27,7 @@ struct apq8016_sbc_data { void __iomem *spkr_iomux; struct snd_soc_jack jack; bool jack_setup; + int mi2s_clk_count[MI2S_COUNT]; }; #define MIC_CTRL_TER_WS_SLAVE_SEL BIT(21) @@ -38,10 +42,10 @@ struct apq8016_sbc_data { #define SPKR_CTL_TLMM_WS_EN_SEL_MASK GENMASK(19, 18) #define SPKR_CTL_TLMM_WS_EN_SEL_SEC BIT(18) #define DEFAULT_MCLK_RATE 9600000 +#define MI2S_BCLK_RATE 1536000 -static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd) +static int apq8016_dai_init(struct snd_soc_pcm_runtime *rtd, int mi2s) { - struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); struct snd_soc_dai *codec_dai; struct snd_soc_component *component; struct snd_soc_card *card = rtd->card; @@ -49,7 +53,7 @@ static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd) int i, rval; u32 value; - switch (cpu_dai->id) { + switch (mi2s) { case MI2S_PRIMARY: writel(readl(pdata->spkr_iomux) | SPKR_CTL_PRI_WS_SLAVE_SEL_11, pdata->spkr_iomux); @@ -128,6 +132,13 @@ static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd) return 0; } +static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd) +{ + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + + return apq8016_dai_init(rtd, cpu_dai->id); +} + static void apq8016_sbc_add_ops(struct snd_soc_card *card) { struct snd_soc_dai_link *link; @@ -137,6 +148,113 @@ static void apq8016_sbc_add_ops(struct snd_soc_card *card) link->init = apq8016_sbc_dai_init; } +static int qdsp6_dai_get_lpass_id(struct snd_soc_dai *cpu_dai) +{ + switch (cpu_dai->id) { + case PRIMARY_MI2S_RX: + case PRIMARY_MI2S_TX: + return MI2S_PRIMARY; + case SECONDARY_MI2S_RX: + case SECONDARY_MI2S_TX: + return MI2S_SECONDARY; + case TERTIARY_MI2S_RX: + case TERTIARY_MI2S_TX: + return MI2S_TERTIARY; + case QUATERNARY_MI2S_RX: + case QUATERNARY_MI2S_TX: + return MI2S_QUATERNARY; + default: + return -EINVAL; + } +} + +static int msm8916_qdsp6_dai_init(struct snd_soc_pcm_runtime *rtd) +{ + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + + snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_CBS_CFS); + return apq8016_dai_init(rtd, qdsp6_dai_get_lpass_id(cpu_dai)); +} + +static int msm8916_qdsp6_startup(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_card *card = rtd->card; + struct apq8016_sbc_data *data = snd_soc_card_get_drvdata(card); + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + int mi2s, ret; + + mi2s = qdsp6_dai_get_lpass_id(cpu_dai); + if (mi2s < 0) + return mi2s; + + if (++data->mi2s_clk_count[mi2s] > 1) + return 0; + + ret = snd_soc_dai_set_sysclk(cpu_dai, LPAIF_BIT_CLK, MI2S_BCLK_RATE, 0); + if (ret) + dev_err(card->dev, "Failed to enable LPAIF bit clk: %d\n", ret); + return ret; +} + +static void msm8916_qdsp6_shutdown(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_card *card = rtd->card; + struct apq8016_sbc_data *data = snd_soc_card_get_drvdata(card); + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + int mi2s, ret; + + mi2s = qdsp6_dai_get_lpass_id(cpu_dai); + if (mi2s < 0) + return; + + if (--data->mi2s_clk_count[mi2s] > 0) + return; + + ret = snd_soc_dai_set_sysclk(cpu_dai, LPAIF_BIT_CLK, 0, 0); + if (ret) + dev_err(card->dev, "Failed to disable LPAIF bit clk: %d\n", ret); +} + +static const struct snd_soc_ops msm8916_qdsp6_be_ops = { + .startup = msm8916_qdsp6_startup, + .shutdown = msm8916_qdsp6_shutdown, +}; + +static int msm8916_qdsp6_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd, + struct snd_pcm_hw_params *params) +{ + struct snd_interval *rate = hw_param_interval(params, + SNDRV_PCM_HW_PARAM_RATE); + struct snd_interval *channels = hw_param_interval(params, + SNDRV_PCM_HW_PARAM_CHANNELS); + struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); + + rate->min = rate->max = 48000; + channels->min = channels->max = 2; + snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE); + + return 0; +} + +static void msm8916_qdsp6_add_ops(struct snd_soc_card *card) +{ + struct snd_soc_dai_link *link; + int i; + + /* Make it obvious to userspace that QDSP6 is used */ + card->components = "qdsp6"; + + for_each_card_prelinks(card, i, link) { + if (link->no_pcm) { + link->init = msm8916_qdsp6_dai_init; + link->ops = &msm8916_qdsp6_be_ops; + link->be_hw_params_fixup = msm8916_qdsp6_be_hw_params_fixup; + } + } +} + static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = { SND_SOC_DAPM_MIC("Handset Mic", NULL), @@ -148,11 +266,16 @@ static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = { static int apq8016_sbc_platform_probe(struct platform_device *pdev) { + void (*add_ops)(struct snd_soc_card *card); struct device *dev = &pdev->dev; struct snd_soc_card *card; struct apq8016_sbc_data *data; int ret; + add_ops = device_get_match_data(&pdev->dev); + if (!add_ops) + return -EINVAL; + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); if (!data) return -ENOMEM; @@ -177,12 +300,13 @@ static int apq8016_sbc_platform_probe(struct platform_device *pdev) snd_soc_card_set_drvdata(card, data); - apq8016_sbc_add_ops(card); + add_ops(card); return devm_snd_soc_register_card(&pdev->dev, card); } static const struct of_device_id apq8016_sbc_device_id[] __maybe_unused = { - { .compatible = "qcom,apq8016-sbc-sndcard" }, + { .compatible = "qcom,apq8016-sbc-sndcard", .data = apq8016_sbc_add_ops }, + { .compatible = "qcom,msm8916-qdsp6-sndcard", .data = msm8916_qdsp6_add_ops }, {}, }; MODULE_DEVICE_TABLE(of, apq8016_sbc_device_id);
The apq8016-sbc-sndcard is designed to be used with the LPASS drivers (bypassing the combined audio/modem DSP in MSM8916/APQ8016). Make it possible to use QDSP6 audio instead for the msm8916-qdsp6-sndcard. This only requires adding some additional hooks that set up the DPCM backends correctly. Similar code is already used in drivers for newer SoCs such as apq8096.c, sdm845.c and sm8250.c. A slightly different initialization sequence is used for the apq8016-sbc and msm8916-qdsp6 sound card by defining the apq8016_sbc_add_ops() function as device match data. Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Signed-off-by: Stephan Gerhold <stephan@gerhold.net> --- sound/soc/qcom/apq8016_sbc.c | 134 +++++++++++++++++++++++++++++++++-- 1 file changed, 129 insertions(+), 5 deletions(-)