Re: [PATCH v2 6/6] remoterproc: qcom: refactor to leverage exported minidump symbol

[Srinivas Kandagatla]

On 14/04/2023 12:49, Mukesh Ojha wrote:
> On 4/14/2023 5:10 PM, Srinivas Kandagatla wrote:
> > On 14/04/2023 12:14, Mukesh Ojha wrote:
> > > On 4/14/2023 4:14 PM, Srinivas Kandagatla wrote:
> > > > On 22/03/2023 13:30, Mukesh Ojha wrote:
> > > > > qcom_minidump driver provides qcom_minidump_subsystem_desc()
> > > > > exported API which other driver can use it query subsystem
> > > > > descriptor. Refactor qcom_minidump() to use this symbol.
> > > > >
> > > > > Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> > > > > ---
> > > > >   drivers/remoteproc/qcom_common.c | 13 ++-----------
> > > > >   1 file changed, 2 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/remoteproc/qcom_common.c 
> > > > > b/drivers/remoteproc/qcom_common.c
> > > > > index 88fc984..240e9f7 100644
> > > > > --- a/drivers/remoteproc/qcom_common.c
> > > > > +++ b/drivers/remoteproc/qcom_common.c
> > > > > @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, 
> > > > > unsigned int minidump_id,
> > > > >   {
> > > > >       int ret;
> > > > >       struct minidump_subsystem *subsystem;
> > > > > -    struct minidump_global_toc *toc;
> > > > > -    /* Get Global minidump ToC*/
> > > > > -    toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, 
> > > > > NULL);
> > > > > -
> > > > > -    /* check if global table pointer exists and init is set */
> > > > > -    if (IS_ERR(toc) || !toc->status) {
> > > > > -        dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
> > > > > +    subsystem = qcom_minidump_subsystem_desc(minidump_id);
> > > > > +    if (IS_ERR(subsystem))
> > > > >           return;
> > > >
> > > > Sorry If I am missing something but I got lost looking at the below 
> > > > code snippet in drivers/remoteproc/qcom_common.c
> > > >
> > > >
> > > > -------------------->cut<-----------------------------
> > > >      subsystem = qcom_minidump_subsystem_desc(minidump_id);
> > > >      if (IS_ERR(subsystem))
> > > >          return;
> > > >
> > > >      /**
> > > >       * Collect minidump if SS ToC is valid and segment table
> > > >       * is initialized in memory and encryption status is set.
> > > >       */
> > > >      if (subsystem->regions_baseptr == 0 ||
> > > >          le32_to_cpu(subsystem->status) != 1 ||
> > > >          le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
> > > >          le32_to_cpu(subsystem->encryption_status) != 
> > > > MINIDUMP_SS_ENCR_DONE) {
> > > >          dev_err(&rproc->dev, "Minidump not ready, skipping\n");
> > > >          return;
> > > >      }
> > > > -------------------->cut<-----------------------------
> > > >
> > > > where does "subsystem->regions_baseptr" for this ADSP minidump 
> > > > descriptor get set?
> > >
> > > Other co-processor such as adsp/cdsp/Mpss has their own way of 
> > > registering their region/segment (mostly they are static known
> > > regions) with minidump global infra and which could be happening
> > > from firmware side .
> > If its happening from firmware side, then that ram phys address range 
> > should be reserved from kernel usage I guess.
> >
> > Do you have more details on where exactly is this reserved from within 
> > linux kernel?
>
> These regions are inside remoteproc memory carve-out.
> like.
>
> adsp_mem: memory@85e00000 {
>      reg = <0x0 0x85e00000 0x0 0x2100000>;
>      no-map;
> };

thanks for explaining.

--srini
> remoteproc_adsp: remoteproc@30000000 {
>      compatible = "qcom,sm8450-adsp-pas";
>      reg = <0 0x30000000 0 0x100>;
>             ...
>             ...
>      memory-region = <&adsp_mem>; <==
>
> -Mukesh
>
> > --srini
> >
> > > -Mukesh
> > >
> > > > --srini
> > > >
> > > > > -    }
> > > > > -
> > > > > -    /* Get subsystem table of contents using the minidump id */
> > > > > -    subsystem = &toc->subsystems[minidump_id];
> > > > >       /**
> > > > >        * Collect minidump if SS ToC is valid and segment table