MIME-Version: 1.0
In-Reply-To: <1637250620-8926-4-git-send-email-pillair@codeaurora.org>
References: <1637250620-8926-1-git-send-email-pillair@codeaurora.org> <1637250620-8926-4-git-send-email-pillair@codeaurora.org>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.9.1
Date:   Fri, 19 Nov 2021 01:54:49 +0100
Message-ID: <CAE-0n5371rNqs6+_ZRtPDqOb7WCrzXUHbxGMjPAdVeLsGgX8_w@mail.gmail.com>
Subject: Re: [PATCH v9 3/3] remoteproc: qcom: q6v5_wpss: Add support for
 sc7280 WPSS
To:     Rakesh Pillai <pillair@codeaurora.org>, agross@kernel.org,
        bjorn.andersson@linaro.org, mathieu.poirier@linaro.org,
        ohad@wizery.com, p.zabel@pengutronix.de, robh+dt@kernel.org
Cc:     linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        sibis@codeaurora.org, mpubbise@codeaurora.org, kuabhs@chromium.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Rakesh Pillai (2021-11-18 07:50:20)
> Add support for PIL loading of WPSS processor for SC7280
> - WPSS boot will be requested by the wifi driver and hence
>   disable auto-boot for WPSS.
> - Add a separate shutdown sequence handler for WPSS.
> - Add multiple power-domain voting support
> - Parse firmware-name from dtsi entry
>
> Signed-off-by: Rakesh Pillai <pillair@codeaurora.org>
> ---

Just a couple nitpicks. Otherwise looks good to me.

>  drivers/remoteproc/qcom_q6v5_adsp.c | 222 +++++++++++++++++++++++++++++++++---
>  1 file changed, 206 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
> index 098362e6..34a6b73 100644
> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
> @@ -32,6 +32,7 @@
>
>  /* time out value */
>  #define ACK_TIMEOUT                    1000
> +#define ACK_TIMEOUT_US                 1000000
>  #define BOOT_FSM_TIMEOUT               10000
>  /* mask values */
>  #define EVB_MASK                       GENMASK(27, 4)
> @@ -51,6 +52,8 @@
>  #define QDSP6SS_CORE_CBCR      0x20
>  #define QDSP6SS_SLEEP_CBCR     0x3c
>
> +#define QCOM_Q6V5_RPROC_PROXY_PD_MAX   3
> +
>  struct adsp_pil_data {
>         int crash_reason_smem;
>         const char *firmware_name;
> @@ -58,9 +61,13 @@ struct adsp_pil_data {
>         const char *ssr_name;
>         const char *sysmon_name;
>         int ssctl_id;
> +       bool is_wpss;
> +       bool auto_boot;
>
>         const char **clk_ids;
>         int num_clks;
> +       const char **proxy_pd_names;
> +       const char *load_state;
>  };
>
>  struct qcom_adsp {
> @@ -93,11 +100,146 @@ struct qcom_adsp {
>         void *mem_region;
>         size_t mem_size;
>
> +       struct device *proxy_pds[QCOM_Q6V5_RPROC_PROXY_PD_MAX];
> +       size_t proxy_pd_count;
> +
>         struct qcom_rproc_glink glink_subdev;
>         struct qcom_rproc_ssr ssr_subdev;
>         struct qcom_sysmon *sysmon;
> +
> +       int (*shutdown)(struct qcom_adsp *adsp);
>  };
>
> +static int qcom_rproc_pds_attach(struct device *dev, struct device **devs,

Can 'devs' be replaced by 'struct qcom_adsp'? And then we can compare
the size against ARRAY_SIZE(adsp->proxy_pds) instead of the #define.

> +                                const char **pd_names)
> +{
> +       size_t num_pds = 0;
> +       int ret;
> +       int i;
> +
> +       if (!pd_names)
> +               return 0;
> +
> +       /* Handle single power domain */
> +       if (dev->pm_domain) {
> +               devs[0] = dev;
> +               pm_runtime_enable(dev);
> +               return 1;
> +       }
> +
> +       while (pd_names[num_pds])
> +               num_pds++;
> +
> +       if (num_pds > QCOM_Q6V5_RPROC_PROXY_PD_MAX)
> +               return -E2BIG;
> +
> +       for (i = 0; i < num_pds; i++) {
> +               devs[i] = dev_pm_domain_attach_by_name(dev, pd_names[i]);
> +               if (IS_ERR_OR_NULL(devs[i])) {
> +                       ret = PTR_ERR(devs[i]) ? : -ENODATA;
> +                       goto unroll_attach;
> +               }
> +       }
> +
> +       return num_pds;
> +
> +unroll_attach:
> +       for (i--; i >= 0; i--)
> +               dev_pm_domain_detach(devs[i], false);
> +
> +       return ret;
> +}
> +
> +static void qcom_rproc_pds_detach(struct qcom_adsp *adsp, struct device **pds,
> +                                 size_t pd_count)
> +{
> +       struct device *dev = adsp->dev;
> +       int i;
> +
> +       /* Handle single power domain */
> +       if (dev->pm_domain && pd_count) {
> +               pm_runtime_disable(dev);
> +               return;
> +       }
> +
> +       for (i = 0; i < pd_count; i++)
> +               dev_pm_domain_detach(pds[i], false);
> +}
> +
> +static int qcom_rproc_pds_enable(struct qcom_adsp *adsp, struct device **pds,
> +                                size_t pd_count)
> +{
> +       int ret;
> +       int i;
> +
> +       for (i = 0; i < pd_count; i++) {
> +               dev_pm_genpd_set_performance_state(pds[i], INT_MAX);
> +               ret = pm_runtime_get_sync(pds[i]);
> +               if (ret < 0) {
> +                       pm_runtime_put_noidle(pds[i]);
> +                       dev_pm_genpd_set_performance_state(pds[i], 0);
> +                       goto unroll_pd_votes;
> +               }
> +       }
> +
> +       return 0;
> +
> +unroll_pd_votes:
> +       for (i--; i >= 0; i--) {
> +               dev_pm_genpd_set_performance_state(pds[i], 0);
> +               pm_runtime_put(pds[i]);
> +       }
> +
> +       return ret;
> +}
> +
> +static void qcom_rproc_pds_disable(struct qcom_adsp *adsp, struct device **pds,
> +                                  size_t pd_count)
> +{
> +       int i;
> +
> +       for (i = 0; i < pd_count; i++) {
> +               dev_pm_genpd_set_performance_state(pds[i], 0);
> +               pm_runtime_put(pds[i]);
> +       }
> +}
> +
> +static int qcom_wpss_shutdown(struct qcom_adsp *adsp)
> +{
> +       unsigned int val;
> +
> +       regmap_write(adsp->halt_map, adsp->halt_lpass + LPASS_HALTREQ_REG, 1);
> +
> +       /* Wait for halt ACK from QDSP6 */
> +       regmap_read_poll_timeout(adsp->halt_map,
> +                                adsp->halt_lpass + LPASS_HALTACK_REG, val,
> +                                val, 1000, ACK_TIMEOUT_US);
> +
> +       /* Assert the WPSS PDC Reset */
> +       reset_control_assert(adsp->pdc_sync_reset);
> +       /* Place the WPSS processor into reset */
> +       reset_control_assert(adsp->restart);
> +       /* wait after asserting subsystem restart from AOSS */
> +       usleep_range(200, 205);
> +       /* Remove the WPSS reset */
> +       reset_control_deassert(adsp->restart);
> +       /* De-assert the WPSS PDC Reset */
> +       reset_control_deassert(adsp->pdc_sync_reset);

Please add newlines between comments and previous code. The above chunk
is really hard to read.

> +
> +       usleep_range(100, 105);
> +
> +       clk_bulk_disable_unprepare(adsp->num_clks, adsp->clks);
> +
> +       regmap_write(adsp->halt_map, adsp->halt_lpass + LPASS_HALTREQ_REG, 0);
> +
> +       /* Wait for halt ACK from QDSP6 */
> +       regmap_read_poll_timeout(adsp->halt_map,
> +                                adsp->halt_lpass + LPASS_HALTACK_REG, val,
> +                                !val, 1000, ACK_TIMEOUT_US);
> +
> +       return 0;
> +}
> +
>  static int qcom_adsp_shutdown(struct qcom_adsp *adsp)
>  {
>         unsigned long timeout;