Re: [PATCH v9 2/8] remoteproc: qcom: Add secure PIL support

[Gokul Sriram P]

On 6/22/2024 2:47 AM, Dmitry Baryshkov wrote:
> On Fri, Jun 21, 2024 at 05:16:53PM GMT, Gokul Sriram Palanisamy wrote:
> > IPQ8074 uses secure PIL. Hence, adding the support for the same.
> See Documentation/process/submitting-patches.rst
  Thanks. Will add detailed description.
> > Signed-off-by: Nikhil Prakash V <quic_nprakash@xxxxxxxxxxx>
> > Signed-off-by: Sricharan R <quic_srichara@xxxxxxxxxxx>
> > Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@xxxxxxxxxxx>
> > ---
> >   drivers/remoteproc/qcom_q6v5_wcss.c | 43 +++++++++++++++++++++++++++--
> >   1 file changed, 40 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
> > index 366b19cbd994..e45e79d80238 100644
> > --- a/drivers/remoteproc/qcom_q6v5_wcss.c
> > +++ b/drivers/remoteproc/qcom_q6v5_wcss.c
> > @@ -18,6 +18,7 @@
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/reset.h>
> >   #include <linux/soc/qcom/mdt_loader.h>
> > +#include <linux/firmware/qcom/qcom_scm.h>
> >   #include "qcom_common.h"
> >   #include "qcom_pil_info.h"
> >   #include "qcom_q6v5.h"
> > @@ -86,6 +87,9 @@
> >   #define TCSR_WCSS_CLK_ENABLE	0x14
> >   
> >   #define MAX_HALT_REG		3
> > +
> > +#define WCNSS_PAS_ID		6
> > +
> >   enum {
> >   	WCSS_IPQ8074,
> >   	WCSS_QCS404,
> > @@ -134,6 +138,7 @@ struct q6v5_wcss {
> >   	unsigned int crash_reason_smem;
> >   	u32 version;
> >   	bool requires_force_stop;
> > +	bool need_mem_protection;
> needs
>
> >   
> >   	struct qcom_rproc_glink glink_subdev;
> >   	struct qcom_rproc_ssr ssr_subdev;
> > @@ -152,6 +157,7 @@ struct wcss_data {
> >   	int ssctl_id;
> >   	const struct rproc_ops *ops;
> >   	bool requires_force_stop;
> > +	bool need_mem_protection;
> needs
  Thanks. Will update.
> >   };
> >   
> >   static int q6v5_wcss_reset(struct q6v5_wcss *wcss)
> > @@ -251,6 +257,15 @@ static int q6v5_wcss_start(struct rproc *rproc)
> >   
> >   	qcom_q6v5_prepare(&wcss->q6v5);
> >   
> > +	if (wcss->need_mem_protection) {
> > +		ret = qcom_scm_pas_auth_and_reset(WCNSS_PAS_ID);
> > +		if (ret) {
> > +			dev_err(wcss->dev, "wcss_reset failed\n");
> > +			return ret;
> > +		}
> > +		goto wait_for_reset;
> > +	}
> Use if/else instead of a goto.

  sure. Will update.

> > +
> >   	/* Release Q6 and WCSS reset */
> >   	ret = reset_control_deassert(wcss->wcss_reset);
> >   	if (ret) {
> > @@ -285,6 +300,7 @@ static int q6v5_wcss_start(struct rproc *rproc)
> >   	if (ret)
> >   		goto wcss_q6_reset;
> >   
> > +wait_for_reset:
> This is more like wait_for_start
  yes, this looks more appropriate. Will update. Thanks.
> >   	ret = qcom_q6v5_wait_for_start(&wcss->q6v5, 5 * HZ);
> >   	if (ret == -ETIMEDOUT)
> >   		dev_err(wcss->dev, "start timed out\n");
> > @@ -718,6 +734,15 @@ static int q6v5_wcss_stop(struct rproc *rproc)
> >   	struct q6v5_wcss *wcss = rproc->priv;
> >   	int ret;
> >   
> > +	if (wcss->need_mem_protection) {
> > +		ret = qcom_scm_pas_shutdown(WCNSS_PAS_ID);
> > +		if (ret) {
> > +			dev_err(wcss->dev, "not able to shutdown\n");
> > +			return ret;
> > +		}
> > +		goto pas_done;
> > +	}
> if/else. Or abstract this to functions.
  sure. Will update appropriately.
> > +
> >   	/* WCSS powerdown */
> >   	if (wcss->requires_force_stop) {
> >   		ret = qcom_q6v5_request_stop(&wcss->q6v5, NULL);
> > @@ -742,6 +767,7 @@ static int q6v5_wcss_stop(struct rproc *rproc)
> >   			return ret;
> >   	}
> >   
> > +pas_done:
> >   	clk_disable_unprepare(wcss->prng_clk);
> >   	qcom_q6v5_unprepare(&wcss->q6v5);
> >   
> > @@ -765,9 +791,15 @@ static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw)
> >   	struct q6v5_wcss *wcss = rproc->priv;
> >   	int ret;
> >   
> > -	ret = qcom_mdt_load_no_init(wcss->dev, fw, rproc->firmware,
> > -				    0, wcss->mem_region, wcss->mem_phys,
> > -				    wcss->mem_size, &wcss->mem_reloc);
> > +	if (wcss->need_mem_protection)
> > +		ret = qcom_mdt_load(wcss->dev, fw, rproc->firmware,
> > +				    WCNSS_PAS_ID, wcss->mem_region,
> > +				    wcss->mem_phys, wcss->mem_size,
> > +				    &wcss->mem_reloc);
> > +	else
> > +		ret = qcom_mdt_load_no_init(wcss->dev, fw, rproc->firmware,
> > +					    0, wcss->mem_region, wcss->mem_phys,
> > +					    wcss->mem_size, &wcss->mem_reloc);
> >   	if (ret)
> >   		return ret;
> >   
> > @@ -1035,6 +1067,9 @@ static int q6v5_wcss_probe(struct platform_device *pdev)
> >   	if (!desc)
> >   		return -EINVAL;
> >   
> > +	if (desc->need_mem_protection && !qcom_scm_is_available())
> > +		return -EPROBE_DEFER;
> > +
> >   	rproc = devm_rproc_alloc(&pdev->dev, pdev->name, desc->ops,
> >   				 desc->firmware_name, sizeof(*wcss));
> >   	if (!rproc) {
> > @@ -1048,6 +1083,7 @@ static int q6v5_wcss_probe(struct platform_device *pdev)
> >   
> >   	wcss->version = desc->version;
> >   	wcss->requires_force_stop = desc->requires_force_stop;
> > +	wcss->need_mem_protection = desc->need_mem_protection;
> >   
> >   	ret = q6v5_wcss_init_mmio(wcss, pdev);
> >   	if (ret)
> > @@ -1111,6 +1147,7 @@ static const struct wcss_data wcss_ipq8074_res_init = {
> >   	.wcss_q6_reset_required = true,
> >   	.ops = &q6v5_wcss_ipq8074_ops,
> >   	.requires_force_stop = true,
> > +	.need_mem_protection = true,
> >   };
> >   
> >   static const struct wcss_data wcss_qcs404_res_init = {
> > --
> > 2.34.1