| Message ID | 20220228172528.3489-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | soundwire: qcom: add pm runtime support | expand |
Hi Srinivas, On Mon, Feb 28, 2022 at 7:13 PM Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > Add support to runtime PM using SoundWire clock stop Mode0 on supported > controller instances and soft reset on instances that do not support > clock stop. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Thanks for your patch, which is now commit 74e79da9fd46a597 ("soundwire: qcom: add runtime pm support") in next-20220303. > --- a/drivers/soundwire/qcom.c > +++ b/drivers/soundwire/qcom.c > @@ -1345,6 +1399,105 @@ static int qcom_swrm_remove(struct platform_device *pdev) > return 0; > } > > +static bool swrm_wait_for_frame_gen_enabled(struct qcom_swrm_ctrl *swrm) > +{ [...] > +} > + > +static int __maybe_unused swrm_runtime_suspend(struct device *dev) This is causing build failures if CONFIG_PM=n: drivers/soundwire/qcom.c:1460:12: error: 'swrm_runtime_resume' defined but not used [-Werror=unused-function] http://kisskb.ellerman.id.au/kisskb/buildresult/14704362/ > +{ [...] > +} > + > +static const struct dev_pm_ops swrm_dev_pm_ops = { > + SET_RUNTIME_PM_OPS(swrm_runtime_suspend, swrm_runtime_resume, NULL) > +}; > + > static const struct of_device_id qcom_swrm_of_match[] = { > { .compatible = "qcom,soundwire-v1.3.0", .data = &swrm_v1_3_data }, > { .compatible = "qcom,soundwire-v1.5.1", .data = &swrm_v1_5_data }, Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, 2 Mar 2022 at 21:13, Vinod Koul <vkoul@kernel.org> wrote: > > On 28-02-22, 17:25, Srinivas Kandagatla wrote: > > This patchset adds pm runtime support to Qualcomm SounWire Controller using > > SoundWire Clock Stop and Wake up using Headset events on supported instances and > > instances like WSA which do not support clock stop a soft reset of controller > > along with full rest of slaves is done to resume from a low power state. > > > > Tested it on SM8250 MTP and Dragon Board DB845c > > Applied, thanks > Hi, this patch series broke audio on SDM845 running AOSP. I can reproduce it on both DB845c and PocoF1 https://www.toptal.com/developers/hastebin/raw/rodazupayu. It is not 100% reproducible but can be triggered on every alternate reboot or so. Regards, Amit Pundir > -- > ~Vinod
> @@ -1017,6 +1032,15 @@ static int qcom_swrm_startup(struct snd_pcm_substream *substream, > struct snd_soc_dai *codec_dai; > int ret, i; > > + ret = pm_runtime_get_sync(ctrl->dev); > + if (ret < 0 && ret != -EACCES) { > + dev_err_ratelimited(ctrl->dev, > + "pm_runtime_get_sync failed in %s, ret %d\n", > + __func__, ret); > + pm_runtime_put_noidle(ctrl->dev); > + return ret; here there's an error handling, but ... > + } > + > sruntime = sdw_alloc_stream(dai->name); > if (!sruntime) > return -ENOMEM; > @@ -1044,6 +1068,9 @@ static void qcom_swrm_shutdown(struct snd_pcm_substream *substream, > > sdw_release_stream(ctrl->sruntime[dai->id]); > ctrl->sruntime[dai->id] = NULL; > + pm_runtime_mark_last_busy(ctrl->dev); > + pm_runtime_put_autosuspend(ctrl->dev); > + > } > > static const struct snd_soc_dai_ops qcom_swrm_pdm_dai_ops = { > @@ -1197,12 +1224,23 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl) > static int swrm_reg_show(struct seq_file *s_file, void *data) > { > struct qcom_swrm_ctrl *swrm = s_file->private; > - int reg, reg_val; > + int reg, reg_val, ret; > + > + ret = pm_runtime_get_sync(swrm->dev); > + if (ret < 0 && ret != -EACCES) { > + dev_err_ratelimited(swrm->dev, > + "pm_runtime_get_sync failed in %s, ret %d\n", > + __func__, ret); > + pm_runtime_put_noidle(swrm->dev); ... here it's missing? I have a fix ready but thought I would check first if this was intentional https://github.com/thesofproject/linux/pull/3602/commits/6353eec8dc971c5f0fda0166ae1777f71784ea32 > + } > > for (reg = 0; reg <= SWR_MSTR_MAX_REG_ADDR; reg += 4) { > swrm->reg_read(swrm, reg, ®_val); > seq_printf(s_file, "0x%.3x: 0x%.2x\n", reg, reg_val); > } > + pm_runtime_mark_last_busy(swrm->dev); > + pm_runtime_put_autosuspend(swrm->dev); > + > > return 0;
>> +static irqreturn_t qcom_swrm_wake_irq_handler(int irq, void *dev_id) >> +{ >> + struct qcom_swrm_ctrl *swrm = dev_id; >> + int ret; >> + >> + ret = pm_runtime_get_sync(swrm->dev); >> + if (ret < 0 && ret != -EACCES) { >> + dev_err_ratelimited(swrm->dev, >> + "pm_runtime_get_sync failed in %s, ret %d\n", >> + __func__, ret); >> + pm_runtime_put_noidle(swrm->dev); missing 'return ret' here as well, is this intentional? Fix at https://github.com/thesofproject/linux/pull/3602/commits/6353eec8dc971c5f0fda0166ae1777f71784ea32 ready to go, but not sure what the intent was. >> + } >> + >> + if (swrm->wake_irq > 0) { >> + if (!irqd_irq_disabled(irq_get_irq_data(swrm->wake_irq))) >> + disable_irq_nosync(swrm->wake_irq); >> + } >> + >> + pm_runtime_mark_last_busy(swrm->dev); >> + pm_runtime_put_autosuspend(swrm->dev); >> + >> + return IRQ_HANDLED; >> +}
On 20/04/2022 18:39, Pierre-Louis Bossart wrote: >> @@ -1197,12 +1224,23 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl) >> static int swrm_reg_show(struct seq_file *s_file, void *data) >> { >> struct qcom_swrm_ctrl *swrm = s_file->private; >> - int reg, reg_val; >> + int reg, reg_val, ret; >> + >> + ret = pm_runtime_get_sync(swrm->dev); >> + if (ret < 0 && ret != -EACCES) { >> + dev_err_ratelimited(swrm->dev, >> + "pm_runtime_get_sync failed in %s, ret %d\n", >> + __func__, ret); >> + pm_runtime_put_noidle(swrm->dev); > ... here it's missing? > > I have a fix ready but thought I would check first if this was intentional Its not intentional. > > https://github.com/thesofproject/linux/pull/3602/commits/6353eec8dc971c5f0fda0166ae1777f71784ea32 Fix looks good. --srini >
>>> @@ -1197,12 +1224,23 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl) >>> static int swrm_reg_show(struct seq_file *s_file, void *data) >>> { >>> struct qcom_swrm_ctrl *swrm = s_file->private; >>> - int reg, reg_val; >>> + int reg, reg_val, ret; >>> + >>> + ret = pm_runtime_get_sync(swrm->dev); >>> + if (ret < 0 && ret != -EACCES) { >>> + dev_err_ratelimited(swrm->dev, >>> + "pm_runtime_get_sync failed in %s, ret %d\n", >>> + __func__, ret); >>> + pm_runtime_put_noidle(swrm->dev); >> ... here it's missing? >> >> I have a fix ready but thought I would check first if this was intentional > > Its not intentional. > > >> >> https://github.com/thesofproject/linux/pull/3602/commits/6353eec8dc971c5f0fda0166ae1777f71784ea32 > > Fix looks good. Thanks Srini, I'll add Fixes tags and share on alsa-devel. I guess we need to thank Mark Brown for his guidance to use pm_runtime_resume_and_get(), that's how I saw those two cases :-)