Message-ID: <CAA8EJpqiv5116E_dW4CB6Az3zuOeQ+nPnOp+icYREjJSSQTX=Q@mail.gmail.com>
Date: Thu, 6 Feb 2025 02:44:09 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Frank Oltmanns <frank@...manns.dev>
Cc: Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	Chris Lew <quic_clew@...cinc.com>, linux-arm-msm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Stephan Gerhold <stephan.gerhold@...aro.org>, 
	Johan Hovold <johan+linaro@...nel.org>, Caleb Connolly <caleb.connolly@...aro.org>, 
	Joel Selvaraj <joelselvaraj.oss@...il.com>, 
	Alexey Minnekhanov <alexeymin@...tmarketos.org>, stable@...r.kernel.org
Subject: Re: [PATCH] soc: qcom: pd-mapper: defer probing on sdm845

On Wed, 5 Feb 2025 at 23:57, Frank Oltmanns <frank@...manns.dev> wrote:
>
> On xiaomi-beryllium and oneplus-enchilada audio does not work reliably
> with the in-kernel pd-mapper. Deferring the probe solves these issues.
> Specifically, audio only works reliably with the in-kernel pd-mapper, if
> the probe succeeds when remoteproc3 triggers the first successful probe.
> I.e., probes from remoteproc0, 1, and 2 need to be deferred until
> remoteproc3 has been probed.

The cause for such issues also must be identified. I mean, in-kernel
pd-mapper uncovered a lot of issues that were not noticed because
usually the starting order was a bit different. But we need to
understand how to actually fix those issues instead of working them
around.

>
> Introduce a device specific quirk that lists the first auxdev for which
> the probe must be executed. Until then, defer probes from other auxdevs.
>
> Fixes: 1ebcde047c54 ("soc: qcom: add pd-mapper implementation")
> Cc: stable@...r.kernel.org
> Signed-off-by: Frank Oltmanns <frank@...manns.dev>
> ---
> The in-kernel pd-mapper has been causing audio issues on sdm845
> devices (specifically, xiaomi-beryllium and oneplus-enchilada). I
> observed that Stephan’s approach [1] - which defers module probing by
> blocklisting the module and triggering a later probe - works reliably.
>
> Inspired by this, I experimented with delaying the probe within the
> module itself by returning -EPROBE_DEFER in qcom_pdm_probe() until a
> certain time (13.9 seconds after boot, based on ktime_get()) had
> elapsed. This method also restored audio functionality.
>
> Further logging of auxdev->id in qcom_pdm_probe() led to an interesting
> discovery: audio only works reliably with the in-kernel pd-mapper when
> the first successful probe is triggered by remoteproc3. In other words,
> probes from remoteproc0, 1, and 2 must be deferred until remoteproc3 has
> been probed.
>
> To address this, I propose introducing a quirk table (which currently
> only contains sdm845) to defer probing until the correct auxiliary
> device (remoteproc3) initiates the probe.
>
> I look forward to your feedback.
>
> Thanks,
>   Frank
>
> [1]: https://lore.kernel.org/linux-arm-msm/Zwj3jDhc9fRoCCn6@linaro.org/
> ---
>  drivers/soc/qcom/qcom_pd_mapper.c | 43 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/drivers/soc/qcom/qcom_pd_mapper.c b/drivers/soc/qcom/qcom_pd_mapper.c
> index 154ca5beb47160cc404a46a27840818fe3187420..34b26df665a888ac4872f56e948e73b561ae3b6b 100644
> --- a/drivers/soc/qcom/qcom_pd_mapper.c
> +++ b/drivers/soc/qcom/qcom_pd_mapper.c
> @@ -46,6 +46,11 @@ struct qcom_pdm_data {
>         struct list_head services;
>  };
>
> +struct qcom_pdm_probe_first_dev_quirk {
> +       const char *name;
> +       u32 id;
> +};
> +
>  static DEFINE_MUTEX(qcom_pdm_mutex); /* protects __qcom_pdm_data */
>  static struct qcom_pdm_data *__qcom_pdm_data;
>
> @@ -526,6 +531,11 @@ static const struct qcom_pdm_domain_data *x1e80100_domains[] = {
>         NULL,
>  };
>
> +static const struct qcom_pdm_probe_first_dev_quirk first_dev_remoteproc3 = {
> +       .id = 3,
> +       .name = "pd-mapper"
> +};
> +
>  static const struct of_device_id qcom_pdm_domains[] __maybe_unused = {
>         { .compatible = "qcom,apq8016", .data = NULL, },
>         { .compatible = "qcom,apq8064", .data = NULL, },
> @@ -566,6 +576,10 @@ static const struct of_device_id qcom_pdm_domains[] __maybe_unused = {
>         {},
>  };
>
> +static const struct of_device_id qcom_pdm_defer[] __maybe_unused = {
> +       { .compatible = "qcom,sdm845", .data = &first_dev_remoteproc3, },
> +       {},
> +};
>  static void qcom_pdm_stop(struct qcom_pdm_data *data)
>  {
>         qcom_pdm_free_domains(data);
> @@ -637,6 +651,25 @@ static struct qcom_pdm_data *qcom_pdm_start(void)
>         return ERR_PTR(ret);
>  }
>
> +static bool qcom_pdm_ready(struct auxiliary_device *auxdev)
> +{
> +       const struct of_device_id *match;
> +       struct device_node *root;
> +       struct qcom_pdm_probe_first_dev_quirk *first_dev;
> +
> +       root = of_find_node_by_path("/");
> +       if (!root)
> +               return true;
> +
> +       match = of_match_node(qcom_pdm_defer, root);
> +       of_node_put(root);
> +       if (!match)
> +               return true;
> +
> +       first_dev = (struct qcom_pdm_probe_first_dev_quirk *) match->data;
> +       return (auxdev->id == first_dev->id) && !strcmp(auxdev->name, first_dev->name);
> +}
> +
>  static int qcom_pdm_probe(struct auxiliary_device *auxdev,
>                           const struct auxiliary_device_id *id)
>
> @@ -647,6 +680,15 @@ static int qcom_pdm_probe(struct auxiliary_device *auxdev,
>         mutex_lock(&qcom_pdm_mutex);
>
>         if (!__qcom_pdm_data) {
> +               if (!qcom_pdm_ready(auxdev)) {
> +                       pr_debug("%s: Deferring probe for device %s (id: %u)\n",
> +                               __func__, auxdev->name, auxdev->id);
> +                       ret = -EPROBE_DEFER;
> +                       goto probe_stop;
> +               }
> +               pr_debug("%s: Probing for device %s (id: %u), starting pdm\n",
> +                       __func__, auxdev->name, auxdev->id);
> +
>                 data = qcom_pdm_start();
>
>                 if (IS_ERR(data))
> @@ -659,6 +701,7 @@ static int qcom_pdm_probe(struct auxiliary_device *auxdev,
>
>         auxiliary_set_drvdata(auxdev, __qcom_pdm_data);
>
> +probe_stop:
>         mutex_unlock(&qcom_pdm_mutex);
>
>         return ret;
>
> ---
> base-commit: 7f048b202333b967782a98aa21bb3354dc379bbf
> change-id: 20250205-qcom_pdm_defer-3dc1271d74d9
>
> Best regards,
> --
> Frank Oltmanns <frank@...manns.dev>
>


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists