Re: [PATCH v7 6/6] remoteproc: qcom: enable in-kernel PD mapper

2024-04-30 Thread Chris Lew 




On 4/26/2024 6:36 PM, Dmitry Baryshkov wrote:

On Sat, 27 Apr 2024 at 04:03, Chris Lew  wrote:




On 4/24/2024 2:28 AM, Dmitry Baryshkov wrote:

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c 
b/drivers/remoteproc/qcom_q6v5_adsp.c
index 1d24c9b656a8..02d0c626b03b 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -23,6 +23,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 

@@ -375,10 +376,14 @@ static int adsp_start(struct rproc *rproc)
   int ret;
   unsigned int val;

- ret = qcom_q6v5_prepare(>q6v5);
+ ret = qcom_pdm_get();
   if (ret)
   return ret;


Would it make sense to try and model this as a rproc subdev? This
section of the remoteproc code seems to be focused on making specific
calls to setup and enable hardware resources, where as pd mapper is
software.

sysmon and ssr are also purely software and they are modeled as subdevs
in qcom_common. I'm not an expert on remoteproc organization but this
was just a thought.


Well, the issue is that the pd-mapper is a global, not a per-remoteproc instance



Both sysmon and ssr have some kind of global states that they manage 
too. Each subdev functionality tends to be a mix of per-remoteproc 
instance management and global state management.


If pd-mapper was completely global, pd-mapper would be able to 
instantiate by itself. Instead, instantiation is dependent on each 
remoteproc instance properly getting and putting references.


The pdm subdev could manage the references to pd-mapper for that 
remoteproc instance.


On the other hand, I think Bjorn recommended this could be moved to 
probe time in v4. The v4 version was doing the reinitialization-dance, 
but I think the recommendation could still apply to this version.




Thanks!
Chris



+ ret = qcom_q6v5_prepare(>q6v5);
+ if (ret)
+ goto put_pdm;
+
   ret = adsp_map_carveout(rproc);
   if (ret) {
   dev_err(adsp->dev, "ADSP smmu mapping failed\n");
@@ -446,6 +451,8 @@ static int adsp_start(struct rproc *rproc)
   adsp_unmap_carveout(rproc);
   disable_irqs:
   qcom_q6v5_unprepare(>q6v5);
+put_pdm:
+ qcom_pdm_release();

   return ret;
   }

Re: [PATCH v7 6/6] remoteproc: qcom: enable in-kernel PD mapper

2024-04-26 Thread Dmitry Baryshkov 
On Sat, 27 Apr 2024 at 04:03, Chris Lew  wrote:
>
>
>
> On 4/24/2024 2:28 AM, Dmitry Baryshkov wrote:
> > diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c 
> > b/drivers/remoteproc/qcom_q6v5_adsp.c
> > index 1d24c9b656a8..02d0c626b03b 100644
> > --- a/drivers/remoteproc/qcom_q6v5_adsp.c
> > +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
> > @@ -23,6 +23,7 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> >   #include 
> >   #include 
> >
> > @@ -375,10 +376,14 @@ static int adsp_start(struct rproc *rproc)
> >   int ret;
> >   unsigned int val;
> >
> > - ret = qcom_q6v5_prepare(>q6v5);
> > + ret = qcom_pdm_get();
> >   if (ret)
> >   return ret;
>
> Would it make sense to try and model this as a rproc subdev? This
> section of the remoteproc code seems to be focused on making specific
> calls to setup and enable hardware resources, where as pd mapper is
> software.
>
> sysmon and ssr are also purely software and they are modeled as subdevs
> in qcom_common. I'm not an expert on remoteproc organization but this
> was just a thought.

Well, the issue is that the pd-mapper is a global, not a per-remoteproc instance

>
> Thanks!
> Chris
>
> >
> > + ret = qcom_q6v5_prepare(>q6v5);
> > + if (ret)
> > + goto put_pdm;
> > +
> >   ret = adsp_map_carveout(rproc);
> >   if (ret) {
> >   dev_err(adsp->dev, "ADSP smmu mapping failed\n");
> > @@ -446,6 +451,8 @@ static int adsp_start(struct rproc *rproc)
> >   adsp_unmap_carveout(rproc);
> >   disable_irqs:
> >   qcom_q6v5_unprepare(>q6v5);
> > +put_pdm:
> > + qcom_pdm_release();
> >
> >   return ret;
> >   }
>


-- 
With best wishes
Dmitry

Re: [PATCH v7 6/6] remoteproc: qcom: enable in-kernel PD mapper

2024-04-26 Thread Chris Lew 




On 4/24/2024 2:28 AM, Dmitry Baryshkov wrote:

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c 
b/drivers/remoteproc/qcom_q6v5_adsp.c
index 1d24c9b656a8..02d0c626b03b 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -375,10 +376,14 @@ static int adsp_start(struct rproc *rproc)

int ret;
unsigned int val;
  
-	ret = qcom_q6v5_prepare(>q6v5);

+   ret = qcom_pdm_get();
if (ret)
return ret;


Would it make sense to try and model this as a rproc subdev? This 
section of the remoteproc code seems to be focused on making specific 
calls to setup and enable hardware resources, where as pd mapper is 
software.


sysmon and ssr are also purely software and they are modeled as subdevs 
in qcom_common. I'm not an expert on remoteproc organization but this 
was just a thought.


Thanks!
Chris

  
+	ret = qcom_q6v5_prepare(>q6v5);

+   if (ret)
+   goto put_pdm;
+
ret = adsp_map_carveout(rproc);
if (ret) {
dev_err(adsp->dev, "ADSP smmu mapping failed\n");
@@ -446,6 +451,8 @@ static int adsp_start(struct rproc *rproc)
adsp_unmap_carveout(rproc);
  disable_irqs:
qcom_q6v5_unprepare(>q6v5);
+put_pdm:
+   qcom_pdm_release();
  
  	return ret;

  }

[PATCH v7 6/6] remoteproc: qcom: enable in-kernel PD mapper

2024-04-24 Thread Dmitry Baryshkov 
Request in-kernel protection domain mapper to be started before starting
Qualcomm DSP and release it once DSP is stopped. Once all DSPs are
stopped, the PD mapper will be stopped too.

Signed-off-by: Dmitry Baryshkov 
---
 drivers/remoteproc/Kconfig  |  4 
 drivers/remoteproc/qcom_q6v5_adsp.c | 11 ++-
 drivers/remoteproc/qcom_q6v5_mss.c  | 10 +-
 drivers/remoteproc/qcom_q6v5_pas.c  | 12 +++-
 drivers/remoteproc/qcom_q6v5_wcss.c | 12 +++-
 5 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 48845dc8fa85..a0ce552f89a1 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -181,6 +181,7 @@ config QCOM_Q6V5_ADSP
depends on QCOM_SYSMON || QCOM_SYSMON=n
depends on RPMSG_QCOM_GLINK || RPMSG_QCOM_GLINK=n
depends on QCOM_AOSS_QMP || QCOM_AOSS_QMP=n
+   depends on QCOM_PD_MAPPER || QCOM_PD_MAPPER=n
select MFD_SYSCON
select QCOM_PIL_INFO
select QCOM_MDT_LOADER
@@ -201,6 +202,7 @@ config QCOM_Q6V5_MSS
depends on QCOM_SYSMON || QCOM_SYSMON=n
depends on RPMSG_QCOM_GLINK || RPMSG_QCOM_GLINK=n
depends on QCOM_AOSS_QMP || QCOM_AOSS_QMP=n
+   depends on QCOM_PD_MAPPER || QCOM_PD_MAPPER=n
select MFD_SYSCON
select QCOM_MDT_LOADER
select QCOM_PIL_INFO
@@ -221,6 +223,7 @@ config QCOM_Q6V5_PAS
depends on QCOM_SYSMON || QCOM_SYSMON=n
depends on RPMSG_QCOM_GLINK || RPMSG_QCOM_GLINK=n
depends on QCOM_AOSS_QMP || QCOM_AOSS_QMP=n
+   depends on QCOM_PD_MAPPER || QCOM_PD_MAPPER=n
select MFD_SYSCON
select QCOM_PIL_INFO
select QCOM_MDT_LOADER
@@ -243,6 +246,7 @@ config QCOM_Q6V5_WCSS
depends on QCOM_SYSMON || QCOM_SYSMON=n
depends on RPMSG_QCOM_GLINK || RPMSG_QCOM_GLINK=n
depends on QCOM_AOSS_QMP || QCOM_AOSS_QMP=n
+   depends on QCOM_PD_MAPPER || QCOM_PD_MAPPER=n
select MFD_SYSCON
select QCOM_MDT_LOADER
select QCOM_PIL_INFO
diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c 
b/drivers/remoteproc/qcom_q6v5_adsp.c
index 1d24c9b656a8..02d0c626b03b 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -375,10 +376,14 @@ static int adsp_start(struct rproc *rproc)
int ret;
unsigned int val;
 
-   ret = qcom_q6v5_prepare(>q6v5);
+   ret = qcom_pdm_get();
if (ret)
return ret;
 
+   ret = qcom_q6v5_prepare(>q6v5);
+   if (ret)
+   goto put_pdm;
+
ret = adsp_map_carveout(rproc);
if (ret) {
dev_err(adsp->dev, "ADSP smmu mapping failed\n");
@@ -446,6 +451,8 @@ static int adsp_start(struct rproc *rproc)
adsp_unmap_carveout(rproc);
 disable_irqs:
qcom_q6v5_unprepare(>q6v5);
+put_pdm:
+   qcom_pdm_release();
 
return ret;
 }
@@ -478,6 +485,8 @@ static int adsp_stop(struct rproc *rproc)
if (handover)
qcom_adsp_pil_handover(>q6v5);
 
+   qcom_pdm_release();
+
return ret;
 }
 
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
b/drivers/remoteproc/qcom_q6v5_mss.c
index 1779fc890e10..791f11e7adbf 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -1581,10 +1582,14 @@ static int q6v5_start(struct rproc *rproc)
int xfermemop_ret;
int ret;
 
-   ret = q6v5_mba_load(qproc);
+   ret = qcom_pdm_get();
if (ret)
return ret;
 
+   ret = q6v5_mba_load(qproc);
+   if (ret)
+   goto put_pdm;
+
dev_info(qproc->dev, "MBA booted with%s debug policy, loading mpss\n",
 qproc->dp_size ? "" : "out");
 
@@ -1613,6 +1618,8 @@ static int q6v5_start(struct rproc *rproc)
 reclaim_mpss:
q6v5_mba_reclaim(qproc);
q6v5_dump_mba_logs(qproc);
+put_pdm:
+   qcom_pdm_release();
 
return ret;
 }
@@ -1627,6 +1634,7 @@ static int q6v5_stop(struct rproc *rproc)
dev_err(qproc->dev, "timed out on wait\n");
 
q6v5_mba_reclaim(qproc);
+   qcom_pdm_release();
 
return 0;
 }
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 54d8005d40a3..653e54f975fc 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -261,10 +262,14 @@ static int adsp_start(struct rproc *rproc)
struct qcom_adsp *adsp = rproc->priv;
int ret;
 
-   ret = qcom_q6v5_prepare(>q6v5);
+   ret = qcom_pdm_get();
if (ret)
return ret;
 
+   ret = qcom_q6v5_prepare(>q6v5);
+   if (ret)
+   goto put_pdm;
+