Re: [PATCH v8 2/9] remoteproc: qcom: Add secure PIL support
On Fri 29 Jan 23:20 CST 2021, Gokul Sriram Palanisamy wrote:
> IPQ8074 uses secure PIL. Hence, adding the support for the same.
>
Sorry for not giving this a proper review before Gokul, I've look at it
but been uncertain about what feedback to offer.
> Signed-off-by: Gokul Sriram Palanisamy
> Signed-off-by: Sricharan R
> Signed-off-by: Nikhil Prakash V
I suspect that there should have some "Co-developed-by" here (and in the
other patches)?
> ---
> 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 c0368bb..4e35e5c 100644
> --- a/drivers/remoteproc/qcom_q6v5_wcss.c
> +++ b/drivers/remoteproc/qcom_q6v5_wcss.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
> #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;
>
> 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;
> };
>
> static int q6v5_wcss_reset(struct q6v5_wcss *wcss)
> @@ -251,6 +257,15 @@ static int q6v5_wcss_start(struct rproc *rproc)
>
> qcom_q6v5_prepare(>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;
This goto essentially puts the entire old function in an "else" block.
> + }
> +
> /* 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:
> ret = qcom_q6v5_wait_for_start(>q6v5, 5 * HZ);
> if (ret == -ETIMEDOUT)
> dev_err(wcss->dev, "start timed out\n");
> @@ -717,6 +733,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;
Same here.
> + }
> +
> /* WCSS powerdown */
> if (wcss->requires_force_stop) {
> ret = qcom_q6v5_request_stop(>q6v5, NULL);
> @@ -741,6 +766,7 @@ static int q6v5_wcss_stop(struct rproc *rproc)
> return ret;
> }
>
> +pas_done:
> clk_disable_unprepare(wcss->prng_clk);
> qcom_q6v5_unprepare(>q6v5);
>
> @@ -764,9 +790,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, >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,
> + >mem_reloc);
> + else
> + ret = qcom_mdt_load_no_init(wcss->dev, fw, rproc->firmware,
> + 0, wcss->mem_region, wcss->mem_phys,
> + wcss->mem_size, >mem_reloc);
And same here.
In practice this means that you're essentially overloading new logic to
all code paths though the driver. Left is some boilerplate code, which I
wish we could refactor into common helper functions in the framework
(e.g. we duplicate q6v5_alloc_memory_region() in a lot of drivers).
So with this in mind, instead of overloading new logic to this entire
driver, could you please submit a new driver for the PAS based IPQ WCSS?
Regards,
Bjorn
> if (ret)
> return ret;
>
> @@ -1032,6 +1064,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;
[PATCH v8 2/9] remoteproc: qcom: Add secure PIL support
IPQ8074 uses secure PIL. Hence, adding the support for the same.
Signed-off-by: Gokul Sriram Palanisamy
Signed-off-by: Sricharan R
Signed-off-by: Nikhil Prakash V
---
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 c0368bb..4e35e5c 100644
--- a/drivers/remoteproc/qcom_q6v5_wcss.c
+++ b/drivers/remoteproc/qcom_q6v5_wcss.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#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;
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;
};
static int q6v5_wcss_reset(struct q6v5_wcss *wcss)
@@ -251,6 +257,15 @@ static int q6v5_wcss_start(struct rproc *rproc)
qcom_q6v5_prepare(>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;
+ }
+
/* 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:
ret = qcom_q6v5_wait_for_start(>q6v5, 5 * HZ);
if (ret == -ETIMEDOUT)
dev_err(wcss->dev, "start timed out\n");
@@ -717,6 +733,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;
+ }
+
/* WCSS powerdown */
if (wcss->requires_force_stop) {
ret = qcom_q6v5_request_stop(>q6v5, NULL);
@@ -741,6 +766,7 @@ static int q6v5_wcss_stop(struct rproc *rproc)
return ret;
}
+pas_done:
clk_disable_unprepare(wcss->prng_clk);
qcom_q6v5_unprepare(>q6v5);
@@ -764,9 +790,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, >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,
+ >mem_reloc);
+ else
+ ret = qcom_mdt_load_no_init(wcss->dev, fw, rproc->firmware,
+ 0, wcss->mem_region, wcss->mem_phys,
+ wcss->mem_size, >mem_reloc);
if (ret)
return ret;
@@ -1032,6 +1064,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 = rproc_alloc(>dev, pdev->name, desc->ops,
desc->firmware_name, sizeof(*wcss));
if (!rproc) {
@@ -1045,6 +1080,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)
@@ -1115,6 +1151,7 @@ static const struct wcss_data wcss_ipq8074_res_init = {
.wcss_q6_reset_required = true,
.ops = _wcss_ipq8074_ops,
.requires_force_stop = true,
+ .need_mem_protection = true,
};
static const struct wcss_data wcss_qcs404_res_init = {
--
2.7.4
