| Message ID | 1679491817-2498-3-git-send-email-quic_mojha@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/6] remoteproc: qcom: Expand MD_* as MINIDUMP_* | expand |
On 22/03/2023 13:30, Mukesh Ojha wrote: > Move minidump specific data types and macros to a separate internal > header(qcom_minidump.h) so that it can be shared among different minidump.h should be good as we are already in include/soc/qcom/ --srini > Qualcomm drivers. > > There is no change in functional behavior after this. > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > drivers/remoteproc/qcom_common.c | 56 +--------------------------------- > include/soc/qcom/qcom_minidump.h | 66 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 67 insertions(+), 55 deletions(-) > create mode 100644 include/soc/qcom/qcom_minidump.h > > diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c > index 805e525..88fc984 100644 > --- a/drivers/remoteproc/qcom_common.c > +++ b/drivers/remoteproc/qcom_common.c > @@ -18,6 +18,7 @@ > #include <linux/slab.h> > #include <linux/soc/qcom/mdt_loader.h> > #include <linux/soc/qcom/smem.h> > +#include <soc/qcom/qcom_minidump.h> > > #include "remoteproc_internal.h" > #include "qcom_common.h" > @@ -26,61 +27,6 @@ > #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev) > #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev) > > -#define MAX_NUM_OF_SS 10 > -#define MAX_REGION_NAME_LENGTH 16 > -#define SBL_MINIDUMP_SMEM_ID 602 > -#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0) > -#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0) > -#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0) > - > -/** > - * struct minidump_region - Minidump region > - * @name : Name of the region to be dumped > - * @seq_num: : Use to differentiate regions with same name. > - * @valid : This entry to be dumped (if set to 1) > - * @address : Physical address of region to be dumped > - * @size : Size of the region > - */ > -struct minidump_region { > - char name[MAX_REGION_NAME_LENGTH]; > - __le32 seq_num; > - __le32 valid; > - __le64 address; > - __le64 size; > -}; > - > -/** > - * struct minidump_subsystem - Subsystem's SMEM Table of content > - * @status : Subsystem toc init status > - * @enabled : if set to 1, this region would be copied during coredump > - * @encryption_status: Encryption status for this subsystem > - * @encryption_required : Decides to encrypt the subsystem regions or not > - * @region_count : Number of regions added in this subsystem toc > - * @regions_baseptr : regions base pointer of the subsystem > - */ > -struct minidump_subsystem { > - __le32 status; > - __le32 enabled; > - __le32 encryption_status; > - __le32 encryption_required; > - __le32 region_count; > - __le64 regions_baseptr; > -}; > - > -/** > - * struct minidump_global_toc - Global Table of Content > - * @status : Global Minidump init status > - * @md_revision : Minidump revision > - * @enabled : Minidump enable status > - * @subsystems : Array of subsystems toc > - */ > -struct minidump_global_toc { > - __le32 status; > - __le32 md_revision; > - __le32 enabled; > - struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; > -}; > - > struct qcom_ssr_subsystem { > const char *name; > struct srcu_notifier_head notifier_list; > diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h > new file mode 100644 > index 0000000..84c8605 > --- /dev/null > +++ b/include/soc/qcom/qcom_minidump.h > @@ -0,0 +1,66 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Qualcomm minidump shared data structures and macros > + * > + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef _QCOM_MINIDUMP_H_ > +#define _QCOM_MINIDUMP_H_ > + > +#define MAX_NUM_OF_SS 10 > +#define MAX_REGION_NAME_LENGTH 16 > +#define SBL_MINIDUMP_SMEM_ID 602 > +#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0) > +#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0) > +#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0) > + > +/** > + * struct minidump_region - Minidump region > + * @name : Name of the region to be dumped > + * @seq_num: : Use to differentiate regions with same name. > + * @valid : This entry to be dumped (if set to 1) > + * @address : Physical address of region to be dumped > + * @size : Size of the region > + */ > +struct minidump_region { > + char name[MAX_REGION_NAME_LENGTH]; > + __le32 seq_num; > + __le32 valid; > + __le64 address; > + __le64 size; > +}; > + > +/** > + * struct minidump_subsystem - Subsystem's SMEM Table of content > + * @status : Subsystem toc init status > + * @enabled : if set to 1, this region would be copied during coredump > + * @encryption_status: Encryption status for this subsystem > + * @encryption_required : Decides to encrypt the subsystem regions or not > + * @region_count : Number of regions added in this subsystem toc > + * @regions_baseptr : regions base pointer of the subsystem > + */ > +struct minidump_subsystem { > + __le32 status; > + __le32 enabled; > + __le32 encryption_status; > + __le32 encryption_required; > + __le32 region_count; > + __le64 regions_baseptr; > +}; > + > +/** > + * struct minidump_global_toc - Global Table of Content > + * @status : Global Minidump init status > + * @md_revision : Minidump revision > + * @enabled : Minidump enable status > + * @subsystems : Array of subsystems toc > + */ > +struct minidump_global_toc { > + __le32 status; > + __le32 md_revision; > + __le32 enabled; > + struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; > +}; > + > +#endif /* _QCOM_MINIDUMP_H_ */
Thanks again for coming back on this. On 4/14/2023 4:02 AM, Srinivas Kandagatla wrote: > > > On 22/03/2023 13:30, Mukesh Ojha wrote: >> Move minidump specific data types and macros to a separate internal >> header(qcom_minidump.h) so that it can be shared among different > > minidump.h should be good as we are already in include/soc/qcom/ Initially, i wanted to protect the content of qcom_minidump.h between qcom_minidump.c and qcom_common.c Ideally, here qcom_minidump.h should be supplier/provider header and can be shared among above qcom_minidump.c and qcom_common.c but since they are not in same directory, moved it inside include/soc/qcom/ as separate header than consumer header minidump.h . -Mukesh > > --srini > >> Qualcomm drivers. >> >> There is no change in functional behavior after this. >> >> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >> --- >> drivers/remoteproc/qcom_common.c | 56 >> +--------------------------------- >> include/soc/qcom/qcom_minidump.h | 66 >> ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 67 insertions(+), 55 deletions(-) >> create mode 100644 include/soc/qcom/qcom_minidump.h >> >> diff --git a/drivers/remoteproc/qcom_common.c >> b/drivers/remoteproc/qcom_common.c >> index 805e525..88fc984 100644 >> --- a/drivers/remoteproc/qcom_common.c >> +++ b/drivers/remoteproc/qcom_common.c >> @@ -18,6 +18,7 @@ >> #include <linux/slab.h> >> #include <linux/soc/qcom/mdt_loader.h> >> #include <linux/soc/qcom/smem.h> >> +#include <soc/qcom/qcom_minidump.h> >> #include "remoteproc_internal.h" >> #include "qcom_common.h" >> @@ -26,61 +27,6 @@ >> #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, >> subdev) >> #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev) >> -#define MAX_NUM_OF_SS 10 >> -#define MAX_REGION_NAME_LENGTH 16 >> -#define SBL_MINIDUMP_SMEM_ID 602 >> -#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << >> 8 | 'I' << 0) >> -#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << >> 8 | 'E' << 0) >> -#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 >> | 'L' << 0) >> - >> -/** >> - * struct minidump_region - Minidump region >> - * @name : Name of the region to be dumped >> - * @seq_num: : Use to differentiate regions with same name. >> - * @valid : This entry to be dumped (if set to 1) >> - * @address : Physical address of region to be dumped >> - * @size : Size of the region >> - */ >> -struct minidump_region { >> - char name[MAX_REGION_NAME_LENGTH]; >> - __le32 seq_num; >> - __le32 valid; >> - __le64 address; >> - __le64 size; >> -}; >> - >> -/** >> - * struct minidump_subsystem - Subsystem's SMEM Table of content >> - * @status : Subsystem toc init status >> - * @enabled : if set to 1, this region would be copied during coredump >> - * @encryption_status: Encryption status for this subsystem >> - * @encryption_required : Decides to encrypt the subsystem regions or >> not >> - * @region_count : Number of regions added in this subsystem toc >> - * @regions_baseptr : regions base pointer of the subsystem >> - */ >> -struct minidump_subsystem { >> - __le32 status; >> - __le32 enabled; >> - __le32 encryption_status; >> - __le32 encryption_required; >> - __le32 region_count; >> - __le64 regions_baseptr; >> -}; >> - >> -/** >> - * struct minidump_global_toc - Global Table of Content >> - * @status : Global Minidump init status >> - * @md_revision : Minidump revision >> - * @enabled : Minidump enable status >> - * @subsystems : Array of subsystems toc >> - */ >> -struct minidump_global_toc { >> - __le32 status; >> - __le32 md_revision; >> - __le32 enabled; >> - struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; >> -}; >> - >> struct qcom_ssr_subsystem { >> const char *name; >> struct srcu_notifier_head notifier_list; >> diff --git a/include/soc/qcom/qcom_minidump.h >> b/include/soc/qcom/qcom_minidump.h >> new file mode 100644 >> index 0000000..84c8605 >> --- /dev/null >> +++ b/include/soc/qcom/qcom_minidump.h >> @@ -0,0 +1,66 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Qualcomm minidump shared data structures and macros >> + * >> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights >> reserved. >> + */ >> + >> +#ifndef _QCOM_MINIDUMP_H_ >> +#define _QCOM_MINIDUMP_H_ >> + >> +#define MAX_NUM_OF_SS 10 >> +#define MAX_REGION_NAME_LENGTH 16 >> +#define SBL_MINIDUMP_SMEM_ID 602 >> +#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << >> 8 | 'I' << 0) >> +#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << >> 8 | 'E' << 0) >> +#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 >> | 'L' << 0) >> + >> +/** >> + * struct minidump_region - Minidump region >> + * @name : Name of the region to be dumped >> + * @seq_num: : Use to differentiate regions with same name. >> + * @valid : This entry to be dumped (if set to 1) >> + * @address : Physical address of region to be dumped >> + * @size : Size of the region >> + */ >> +struct minidump_region { >> + char name[MAX_REGION_NAME_LENGTH]; >> + __le32 seq_num; >> + __le32 valid; >> + __le64 address; >> + __le64 size; >> +}; >> + >> +/** >> + * struct minidump_subsystem - Subsystem's SMEM Table of content >> + * @status : Subsystem toc init status >> + * @enabled : if set to 1, this region would be copied during coredump >> + * @encryption_status: Encryption status for this subsystem >> + * @encryption_required : Decides to encrypt the subsystem regions or >> not >> + * @region_count : Number of regions added in this subsystem toc >> + * @regions_baseptr : regions base pointer of the subsystem >> + */ >> +struct minidump_subsystem { >> + __le32 status; >> + __le32 enabled; >> + __le32 encryption_status; >> + __le32 encryption_required; >> + __le32 region_count; >> + __le64 regions_baseptr; >> +}; >> + >> +/** >> + * struct minidump_global_toc - Global Table of Content >> + * @status : Global Minidump init status >> + * @md_revision : Minidump revision >> + * @enabled : Minidump enable status >> + * @subsystems : Array of subsystems toc >> + */ >> +struct minidump_global_toc { >> + __le32 status; >> + __le32 md_revision; >> + __le32 enabled; >> + struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; >> +}; >> + >> +#endif /* _QCOM_MINIDUMP_H_ */
On 14/04/2023 08:05, Mukesh Ojha wrote: > Thanks again for coming back on this. > > On 4/14/2023 4:02 AM, Srinivas Kandagatla wrote: >> >> >> On 22/03/2023 13:30, Mukesh Ojha wrote: >>> Move minidump specific data types and macros to a separate internal >>> header(qcom_minidump.h) so that it can be shared among different >> >> minidump.h should be good as we are already in include/soc/qcom/ > > > Initially, i wanted to protect the content of qcom_minidump.h between > qcom_minidump.c and qcom_common.c > > Ideally, here qcom_minidump.h should be supplier/provider header and can Am not sure if I understand the supplier concept correctly. AFAIU, we have a 2 sets of apis 1. get hold of minidump descriptor based on minidump_id fro gtoc using qcom_minidump_subsystem_desc(). Am assuming which ever driver uses this api will set there segments and regions in there respective drivers. 2. setting regions/segments in APSS minidump descriptors which are done via qcom_minidump_region_register(). TBH this should be renamed to qcom_apss_minidump_region_register(). mixing of thsee apis makes it bit confusing, specially we have these two category of apis that deal with different things. Does it make sense to spit and abstract them properly by doing? 1. minidump driver to deal with handling gtoc and providing descriptors to the consumers like remoteproc or apss, This can probably live within smem driver as loc for this support is very minimal and proabably rename the api accordingly. 2. apss_minidump driver to allow other qcom drivers to register/unregister regions within apss minidump descriptor. did I miss something? thanks, Srini > be shared among above qcom_minidump.c and qcom_common.c but since they > are not in same directory, moved it inside include/soc/qcom/ as separate > header than consumer header minidump.h . > > -Mukesh >> >> --srini >> >>> Qualcomm drivers. >>> >>> There is no change in functional behavior after this. >>> >>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>> --- >>> drivers/remoteproc/qcom_common.c | 56 >>> +--------------------------------- >>> include/soc/qcom/qcom_minidump.h | 66 >>> ++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 67 insertions(+), 55 deletions(-) >>> create mode 100644 include/soc/qcom/qcom_minidump.h >>> >>> diff --git a/drivers/remoteproc/qcom_common.c >>> b/drivers/remoteproc/qcom_common.c >>> index 805e525..88fc984 100644 >>> --- a/drivers/remoteproc/qcom_common.c >>> +++ b/drivers/remoteproc/qcom_common.c >>> @@ -18,6 +18,7 @@ >>> #include <linux/slab.h> >>> #include <linux/soc/qcom/mdt_loader.h> >>> #include <linux/soc/qcom/smem.h> >>> +#include <soc/qcom/qcom_minidump.h> >>> #include "remoteproc_internal.h" >>> #include "qcom_common.h" >>> @@ -26,61 +27,6 @@ >>> #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, >>> subdev) >>> #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, >>> subdev) >>> -#define MAX_NUM_OF_SS 10 >>> -#define MAX_REGION_NAME_LENGTH 16 >>> -#define SBL_MINIDUMP_SMEM_ID 602 >>> -#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << >>> 8 | 'I' << 0) >>> -#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << >>> 8 | 'E' << 0) >>> -#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 >>> | 'L' << 0) >>> - >>> -/** >>> - * struct minidump_region - Minidump region >>> - * @name : Name of the region to be dumped >>> - * @seq_num: : Use to differentiate regions with same name. >>> - * @valid : This entry to be dumped (if set to 1) >>> - * @address : Physical address of region to be dumped >>> - * @size : Size of the region >>> - */ >>> -struct minidump_region { >>> - char name[MAX_REGION_NAME_LENGTH]; >>> - __le32 seq_num; >>> - __le32 valid; >>> - __le64 address; >>> - __le64 size; >>> -}; >>> - >>> -/** >>> - * struct minidump_subsystem - Subsystem's SMEM Table of content >>> - * @status : Subsystem toc init status >>> - * @enabled : if set to 1, this region would be copied during coredump >>> - * @encryption_status: Encryption status for this subsystem >>> - * @encryption_required : Decides to encrypt the subsystem regions >>> or not >>> - * @region_count : Number of regions added in this subsystem toc >>> - * @regions_baseptr : regions base pointer of the subsystem >>> - */ >>> -struct minidump_subsystem { >>> - __le32 status; >>> - __le32 enabled; >>> - __le32 encryption_status; >>> - __le32 encryption_required; >>> - __le32 region_count; >>> - __le64 regions_baseptr; >>> -}; >>> - >>> -/** >>> - * struct minidump_global_toc - Global Table of Content >>> - * @status : Global Minidump init status >>> - * @md_revision : Minidump revision >>> - * @enabled : Minidump enable status >>> - * @subsystems : Array of subsystems toc >>> - */ >>> -struct minidump_global_toc { >>> - __le32 status; >>> - __le32 md_revision; >>> - __le32 enabled; >>> - struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; >>> -}; >>> - >>> struct qcom_ssr_subsystem { >>> const char *name; >>> struct srcu_notifier_head notifier_list; >>> diff --git a/include/soc/qcom/qcom_minidump.h >>> b/include/soc/qcom/qcom_minidump.h >>> new file mode 100644 >>> index 0000000..84c8605 >>> --- /dev/null >>> +++ b/include/soc/qcom/qcom_minidump.h >>> @@ -0,0 +1,66 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +/* >>> + * Qualcomm minidump shared data structures and macros >>> + * >>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights >>> reserved. >>> + */ >>> + >>> +#ifndef _QCOM_MINIDUMP_H_ >>> +#define _QCOM_MINIDUMP_H_ >>> + >>> +#define MAX_NUM_OF_SS 10 >>> +#define MAX_REGION_NAME_LENGTH 16 >>> +#define SBL_MINIDUMP_SMEM_ID 602 >>> +#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << >>> 8 | 'I' << 0) >>> +#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << >>> 8 | 'E' << 0) >>> +#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 >>> | 'L' << 0) >>> + >>> +/** >>> + * struct minidump_region - Minidump region >>> + * @name : Name of the region to be dumped >>> + * @seq_num: : Use to differentiate regions with same name. >>> + * @valid : This entry to be dumped (if set to 1) >>> + * @address : Physical address of region to be dumped >>> + * @size : Size of the region >>> + */ >>> +struct minidump_region { >>> + char name[MAX_REGION_NAME_LENGTH]; >>> + __le32 seq_num; >>> + __le32 valid; >>> + __le64 address; >>> + __le64 size; >>> +}; >>> + >>> +/** >>> + * struct minidump_subsystem - Subsystem's SMEM Table of content >>> + * @status : Subsystem toc init status >>> + * @enabled : if set to 1, this region would be copied during coredump >>> + * @encryption_status: Encryption status for this subsystem >>> + * @encryption_required : Decides to encrypt the subsystem regions >>> or not >>> + * @region_count : Number of regions added in this subsystem toc >>> + * @regions_baseptr : regions base pointer of the subsystem >>> + */ >>> +struct minidump_subsystem { >>> + __le32 status; >>> + __le32 enabled; >>> + __le32 encryption_status; >>> + __le32 encryption_required; >>> + __le32 region_count; >>> + __le64 regions_baseptr; >>> +}; >>> + >>> +/** >>> + * struct minidump_global_toc - Global Table of Content >>> + * @status : Global Minidump init status >>> + * @md_revision : Minidump revision >>> + * @enabled : Minidump enable status >>> + * @subsystems : Array of subsystems toc >>> + */ >>> +struct minidump_global_toc { >>> + __le32 status; >>> + __le32 md_revision; >>> + __le32 enabled; >>> + struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; >>> +}; >>> + >>> +#endif /* _QCOM_MINIDUMP_H_ */
On 4/14/2023 4:10 PM, Srinivas Kandagatla wrote: > > > On 14/04/2023 08:05, Mukesh Ojha wrote: >> Thanks again for coming back on this. >> >> On 4/14/2023 4:02 AM, Srinivas Kandagatla wrote: >>> >>> >>> On 22/03/2023 13:30, Mukesh Ojha wrote: >>>> Move minidump specific data types and macros to a separate internal >>>> header(qcom_minidump.h) so that it can be shared among different >>> >>> minidump.h should be good as we are already in include/soc/qcom/ >> >> >> Initially, i wanted to protect the content of qcom_minidump.h between >> qcom_minidump.c and qcom_common.c >> >> Ideally, here qcom_minidump.h should be supplier/provider header and can > > Am not sure if I understand the supplier concept correctly. > AFAIU, we have a 2 sets of apis > > 1. get hold of minidump descriptor based on minidump_id fro gtoc using > qcom_minidump_subsystem_desc(). Am assuming which ever driver uses this > api will set there segments and regions in there respective drivers. > > 2. setting regions/segments in APSS minidump descriptors which are done > via qcom_minidump_region_register(). TBH this should be renamed to > qcom_apss_minidump_region_register(). > > mixing of thsee apis makes it bit confusing, specially we have these two > category of apis that deal with different things. > > Does it make sense to spit and abstract them properly by doing? > > > 1. minidump driver to deal with handling gtoc and providing descriptors > to the consumers like remoteproc or apss, This can probably live within > smem driver as loc for this support is very minimal and proabably rename > the api accordingly. > > 2. apss_minidump driver to allow other qcom drivers to > register/unregister regions within apss minidump descriptor. > > did I miss something? No, you are correct with your understanding. To your suggestion to split code and to keep qcom_minidump_subsystem_desc() live inside smem driver, And how about the header qcom_minidump.h, should we keep it separate than the apss minidump client header minidump.h ? Since, you kind of understand the driver now, do you think it is worth to create platform device for minidump from smem driver, and have a probe inside apss minidump driver to solve chicken and egg problem for the clients who comes before minidump and try to register itself without doing ready check and apss_minidump can note this client entry and only register this region once minidump probe success and then it can register all the noted clients in one go. The reason to do this would be apss_minidump driver has no meaning without smem, and for this reason checking qcom_minidump_ready() would not be ideal for at least qcom clients and for core kernel may be. --Mukesh > > thanks, > Srini > >> be shared among above qcom_minidump.c and qcom_common.c but since they >> are not in same directory, moved it inside include/soc/qcom/ as >> separate header than consumer header minidump.h . > >> -Mukesh >>> >>> --srini >>> >>>> Qualcomm drivers. >>>> >>>> There is no change in functional behavior after this. >>>> >>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>>> --- >>>> drivers/remoteproc/qcom_common.c | 56 >>>> +--------------------------------- >>>> include/soc/qcom/qcom_minidump.h | 66 >>>> ++++++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 67 insertions(+), 55 deletions(-) >>>> create mode 100644 include/soc/qcom/qcom_minidump.h >>>> >>>> diff --git a/drivers/remoteproc/qcom_common.c >>>> b/drivers/remoteproc/qcom_common.c >>>> index 805e525..88fc984 100644 >>>> --- a/drivers/remoteproc/qcom_common.c >>>> +++ b/drivers/remoteproc/qcom_common.c >>>> @@ -18,6 +18,7 @@ >>>> #include <linux/slab.h> >>>> #include <linux/soc/qcom/mdt_loader.h> >>>> #include <linux/soc/qcom/smem.h> >>>> +#include <soc/qcom/qcom_minidump.h> >>>> #include "remoteproc_internal.h" >>>> #include "qcom_common.h" >>>> @@ -26,61 +27,6 @@ >>>> #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, >>>> subdev) >>>> #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, >>>> subdev) >>>> -#define MAX_NUM_OF_SS 10 >>>> -#define MAX_REGION_NAME_LENGTH 16 >>>> -#define SBL_MINIDUMP_SMEM_ID 602 >>>> -#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' >>>> << 8 | 'I' << 0) >>>> -#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' >>>> << 8 | 'E' << 0) >>>> -#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << >>>> 8 | 'L' << 0) >>>> - >>>> -/** >>>> - * struct minidump_region - Minidump region >>>> - * @name : Name of the region to be dumped >>>> - * @seq_num: : Use to differentiate regions with same name. >>>> - * @valid : This entry to be dumped (if set to 1) >>>> - * @address : Physical address of region to be dumped >>>> - * @size : Size of the region >>>> - */ >>>> -struct minidump_region { >>>> - char name[MAX_REGION_NAME_LENGTH]; >>>> - __le32 seq_num; >>>> - __le32 valid; >>>> - __le64 address; >>>> - __le64 size; >>>> -}; >>>> - >>>> -/** >>>> - * struct minidump_subsystem - Subsystem's SMEM Table of content >>>> - * @status : Subsystem toc init status >>>> - * @enabled : if set to 1, this region would be copied during coredump >>>> - * @encryption_status: Encryption status for this subsystem >>>> - * @encryption_required : Decides to encrypt the subsystem regions >>>> or not >>>> - * @region_count : Number of regions added in this subsystem toc >>>> - * @regions_baseptr : regions base pointer of the subsystem >>>> - */ >>>> -struct minidump_subsystem { >>>> - __le32 status; >>>> - __le32 enabled; >>>> - __le32 encryption_status; >>>> - __le32 encryption_required; >>>> - __le32 region_count; >>>> - __le64 regions_baseptr; >>>> -}; >>>> - >>>> -/** >>>> - * struct minidump_global_toc - Global Table of Content >>>> - * @status : Global Minidump init status >>>> - * @md_revision : Minidump revision >>>> - * @enabled : Minidump enable status >>>> - * @subsystems : Array of subsystems toc >>>> - */ >>>> -struct minidump_global_toc { >>>> - __le32 status; >>>> - __le32 md_revision; >>>> - __le32 enabled; >>>> - struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; >>>> -}; >>>> - >>>> struct qcom_ssr_subsystem { >>>> const char *name; >>>> struct srcu_notifier_head notifier_list; >>>> diff --git a/include/soc/qcom/qcom_minidump.h >>>> b/include/soc/qcom/qcom_minidump.h >>>> new file mode 100644 >>>> index 0000000..84c8605 >>>> --- /dev/null >>>> +++ b/include/soc/qcom/qcom_minidump.h >>>> @@ -0,0 +1,66 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>>> +/* >>>> + * Qualcomm minidump shared data structures and macros >>>> + * >>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights >>>> reserved. >>>> + */ >>>> + >>>> +#ifndef _QCOM_MINIDUMP_H_ >>>> +#define _QCOM_MINIDUMP_H_ >>>> + >>>> +#define MAX_NUM_OF_SS 10 >>>> +#define MAX_REGION_NAME_LENGTH 16 >>>> +#define SBL_MINIDUMP_SMEM_ID 602 >>>> +#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' >>>> << 8 | 'I' << 0) >>>> +#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' >>>> << 8 | 'E' << 0) >>>> +#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << >>>> 8 | 'L' << 0) >>>> + >>>> +/** >>>> + * struct minidump_region - Minidump region >>>> + * @name : Name of the region to be dumped >>>> + * @seq_num: : Use to differentiate regions with same name. >>>> + * @valid : This entry to be dumped (if set to 1) >>>> + * @address : Physical address of region to be dumped >>>> + * @size : Size of the region >>>> + */ >>>> +struct minidump_region { >>>> + char name[MAX_REGION_NAME_LENGTH]; >>>> + __le32 seq_num; >>>> + __le32 valid; >>>> + __le64 address; >>>> + __le64 size; >>>> +}; >>>> + >>>> +/** >>>> + * struct minidump_subsystem - Subsystem's SMEM Table of content >>>> + * @status : Subsystem toc init status >>>> + * @enabled : if set to 1, this region would be copied during coredump >>>> + * @encryption_status: Encryption status for this subsystem >>>> + * @encryption_required : Decides to encrypt the subsystem regions >>>> or not >>>> + * @region_count : Number of regions added in this subsystem toc >>>> + * @regions_baseptr : regions base pointer of the subsystem >>>> + */ >>>> +struct minidump_subsystem { >>>> + __le32 status; >>>> + __le32 enabled; >>>> + __le32 encryption_status; >>>> + __le32 encryption_required; >>>> + __le32 region_count; >>>> + __le64 regions_baseptr; >>>> +}; >>>> + >>>> +/** >>>> + * struct minidump_global_toc - Global Table of Content >>>> + * @status : Global Minidump init status >>>> + * @md_revision : Minidump revision >>>> + * @enabled : Minidump enable status >>>> + * @subsystems : Array of subsystems toc >>>> + */ >>>> +struct minidump_global_toc { >>>> + __le32 status; >>>> + __le32 md_revision; >>>> + __le32 enabled; >>>> + struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; >>>> +}; >>>> + >>>> +#endif /* _QCOM_MINIDUMP_H_ */
On 17/04/2023 14:22, Mukesh Ojha wrote: > > > On 4/14/2023 4:10 PM, Srinivas Kandagatla wrote: >> >> >> On 14/04/2023 08:05, Mukesh Ojha wrote: >>> Thanks again for coming back on this. >>> >>> On 4/14/2023 4:02 AM, Srinivas Kandagatla wrote: >>>> >>>> >>>> On 22/03/2023 13:30, Mukesh Ojha wrote: >>>>> Move minidump specific data types and macros to a separate internal >>>>> header(qcom_minidump.h) so that it can be shared among different >>>> >>>> minidump.h should be good as we are already in include/soc/qcom/ >>> >>> >>> Initially, i wanted to protect the content of qcom_minidump.h between >>> qcom_minidump.c and qcom_common.c >>> >>> Ideally, here qcom_minidump.h should be supplier/provider header and can >> >> Am not sure if I understand the supplier concept correctly. >> AFAIU, we have a 2 sets of apis >> >> 1. get hold of minidump descriptor based on minidump_id fro gtoc using >> qcom_minidump_subsystem_desc(). Am assuming which ever driver uses >> this api will set there segments and regions in there respective drivers. >> >> 2. setting regions/segments in APSS minidump descriptors which are >> done via qcom_minidump_region_register(). TBH this should be renamed >> to qcom_apss_minidump_region_register(). >> >> mixing of thsee apis makes it bit confusing, specially we have these >> two category of apis that deal with different things. >> >> Does it make sense to spit and abstract them properly by doing? >> >> >> 1. minidump driver to deal with handling gtoc and providing >> descriptors to the consumers like remoteproc or apss, This can >> probably live within smem driver as loc for this support is very >> minimal and proabably rename the api accordingly. >> > >> 2. apss_minidump driver to allow other qcom drivers to >> register/unregister regions within apss minidump descriptor. >> >> did I miss something? > > No, you are correct with your understanding. > > To your suggestion to split code and to keep > qcom_minidump_subsystem_desc() live inside smem driver, > > And how about the header qcom_minidump.h, should we keep it separate > than the apss minidump client header minidump.h ? I tried to do what I suggested and it kinda looked okay w.r.t design but started to look like a bit overdo. other thing that I tried like what you have in patches but creating a dedicated platform driver for minidump (along with apss minidump), just like other smem devices like socinfo.. This should also get rid of qcom_minidump_ready(). > > Since, you kind of understand the driver now, do you think it is worth > to create platform device for minidump from smem driver, and > have a probe inside apss minidump driver to solve chicken and egg > problem for the clients who comes before minidump and try to register > itself without doing ready check and apss_minidump can note this client > entry and only register this region once minidump probe success and then > it can register all the noted clients in one go. yes, it is doable. > > The reason to do this would be apss_minidump driver has no meaning > without smem, and for this reason checking qcom_minidump_ready() would > not be ideal for at least qcom clients and for core kernel may be. > how about something like this: -------------------->cut<----------------------- diff --git a/drivers/soc/qcom/minidump.c b/drivers/soc/qcom/minidump.c new file mode 100644 index 000000000000..4171054b268d --- /dev/null +++ b/drivers/soc/qcom/minidump.c @@ -0,0 +1,100 @@ +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/soc/qcom/smem.h> +#include <linux/soc/qcom/minidump.h> + +... + +struct minidump { + struct minidump_global_toc *md_gbl_toc; + struct minidump_subsystem *md_apss_toc; + ... +}; + +struct minidump *__md; + +struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index) +{ + if (!__md) + return PTR_ERR(-EPROBE_DEFER); + + return &__mdgtoc->subsystems[minidump_index]; +} +EXPORT_SYMBOL(qcom_minidump_subsystem_desc); + + +int qcom_minidump_apss_region_register(const struct qcom_minidump_region *region) +{ + if (!__mdgtoc) + return -EPROBE_DEFER; + ... +} +EXPORT_SYMBOL_GPL(qcom_minidump_apss_region_register); + +static int qcom_minidump_init_apss_subsystem(struct minidumd *md) +{ + struct minidump_subsystem *apsstoc; + + apsstoc = qcom_minidump_subsystem_desc(MINIDUMP_APSS_DESC); + if (IS_ERR(apsstoc)) + return -EINVAL; + + md->md_regions = kcalloc(MAX_NUM_ENTRIES, + sizeof(struct minidump_region), GFP_KERNEL); + if (!md->md_regions) + return -ENOMEM; + + minidump.apss_toc = apsstoc; + + ... + + return 0; +} +static int qcom_minidump_probe(struct platform_device *pdev) +{ + struct minidump *md; + struct minidump_global_toc *mdgtoc; + + md = devm_kzalloc(&pdev->dev, sizeof(*md), GFP_KERNEL); + + ... + mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size); + if (IS_ERR(mdgtoc)) { + dev_err(&pdev->dev, "Couldn't find minidump\n"); + return ERR_PTR(mdgtoc); + } + + if (size < sizeof(*mdgtoc) || !mdgtoc->status) { + dev_err(&pdev->dev, "Minidump table is not initialized\n"); + return ERR_PTR(-EINVAL); + } + ... + qcom_minidump_init_apss_subsystem(md); + + __md = md; + ... + return 0; +} + +static int qcom_minidump_remove(struct platform_device *pdev) +{ + struct qcom_minidump *qs = platform_get_drvdata(pdev); + + ... + + return 0; +} + +static struct platform_driver qcom_minidump_driver = { + .probe = qcom_minidump_probe, + .remove = qcom_minidump_remove, + .driver = { + .name = "qcom-minidump", + }, +}; + +module_platform_driver(qcom_minidump_driver); + +MODULE_DESCRIPTION("Qualcomm minidump driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:qcom-minidump"); diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c index 4f163d62942c..d54b07095ddd 100644 --- a/drivers/soc/qcom/smem.c +++ b/drivers/soc/qcom/smem.c @@ -279,6 +279,7 @@ struct qcom_smem { u32 item_count; struct platform_device *socinfo; + struct platform_device *minidump; struct smem_ptable *ptable; struct smem_partition global_partition; struct smem_partition partitions[SMEM_HOST_COUNT]; @@ -1151,12 +1152,19 @@ static int qcom_smem_probe(struct platform_device *pdev) if (IS_ERR(smem->socinfo)) dev_dbg(&pdev->dev, "failed to register socinfo device\n"); + smem->minidump = platform_device_register_data(&pdev->dev, "qcom-minidump", + PLATFORM_DEVID_NONE, NULL, + 0); + if (IS_ERR(smem->minidump)) + dev_dbg(&pdev->dev, "failed to register minidump device\n"); + return 0; } static int qcom_smem_remove(struct platform_device *pdev) { platform_device_unregister(__smem->socinfo); + platform_device_unregister(__smem->minidump); hwspin_lock_free(__smem->hwlock); __smem = NULL; (END) -------------------->cut<----------------------- > --Mukesh >> >> thanks, >> Srini >> >>> be shared among above qcom_minidump.c and qcom_common.c but since >>> they are not in same directory, moved it inside include/soc/qcom/ as >>> separate header than consumer header minidump.h . > >>> -Mukesh >>>> >>>> --srini >>>> >>>>> Qualcomm drivers. >>>>> >>>>> There is no change in functional behavior after this. >>>>> >>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>>>> --- >>>>> drivers/remoteproc/qcom_common.c | 56 >>>>> +--------------------------------- >>>>> include/soc/qcom/qcom_minidump.h | 66 >>>>> ++++++++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 67 insertions(+), 55 deletions(-) >>>>> create mode 100644 include/soc/qcom/qcom_minidump.h >>>>> >>>>> diff --git a/drivers/remoteproc/qcom_common.c >>>>> b/drivers/remoteproc/qcom_common.c >>>>> index 805e525..88fc984 100644 >>>>> --- a/drivers/remoteproc/qcom_common.c >>>>> +++ b/drivers/remoteproc/qcom_common.c >>>>> @@ -18,6 +18,7 @@ >>>>> #include <linux/slab.h> >>>>> #include <linux/soc/qcom/mdt_loader.h> >>>>> #include <linux/soc/qcom/smem.h> >>>>> +#include <soc/qcom/qcom_minidump.h> >>>>> #include "remoteproc_internal.h" >>>>> #include "qcom_common.h" >>>>> @@ -26,61 +27,6 @@ >>>>> #define to_smd_subdev(d) container_of(d, struct >>>>> qcom_rproc_subdev, subdev) >>>>> #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, >>>>> subdev) >>>>> -#define MAX_NUM_OF_SS 10 >>>>> -#define MAX_REGION_NAME_LENGTH 16 >>>>> -#define SBL_MINIDUMP_SMEM_ID 602 >>>>> -#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' >>>>> << 8 | 'I' << 0) >>>>> -#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' >>>>> << 8 | 'E' << 0) >>>>> -#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << >>>>> 8 | 'L' << 0) >>>>> - >>>>> -/** >>>>> - * struct minidump_region - Minidump region >>>>> - * @name : Name of the region to be dumped >>>>> - * @seq_num: : Use to differentiate regions with same name. >>>>> - * @valid : This entry to be dumped (if set to 1) >>>>> - * @address : Physical address of region to be dumped >>>>> - * @size : Size of the region >>>>> - */ >>>>> -struct minidump_region { >>>>> - char name[MAX_REGION_NAME_LENGTH]; >>>>> - __le32 seq_num; >>>>> - __le32 valid; >>>>> - __le64 address; >>>>> - __le64 size; >>>>> -}; >>>>> - >>>>> -/** >>>>> - * struct minidump_subsystem - Subsystem's SMEM Table of content >>>>> - * @status : Subsystem toc init status >>>>> - * @enabled : if set to 1, this region would be copied during >>>>> coredump >>>>> - * @encryption_status: Encryption status for this subsystem >>>>> - * @encryption_required : Decides to encrypt the subsystem regions >>>>> or not >>>>> - * @region_count : Number of regions added in this subsystem toc >>>>> - * @regions_baseptr : regions base pointer of the subsystem >>>>> - */ >>>>> -struct minidump_subsystem { >>>>> - __le32 status; >>>>> - __le32 enabled; >>>>> - __le32 encryption_status; >>>>> - __le32 encryption_required; >>>>> - __le32 region_count; >>>>> - __le64 regions_baseptr; >>>>> -}; >>>>> - >>>>> -/** >>>>> - * struct minidump_global_toc - Global Table of Content >>>>> - * @status : Global Minidump init status >>>>> - * @md_revision : Minidump revision >>>>> - * @enabled : Minidump enable status >>>>> - * @subsystems : Array of subsystems toc >>>>> - */ >>>>> -struct minidump_global_toc { >>>>> - __le32 status; >>>>> - __le32 md_revision; >>>>> - __le32 enabled; >>>>> - struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; >>>>> -}; >>>>> - >>>>> struct qcom_ssr_subsystem { >>>>> const char *name; >>>>> struct srcu_notifier_head notifier_list; >>>>> diff --git a/include/soc/qcom/qcom_minidump.h >>>>> b/include/soc/qcom/qcom_minidump.h >>>>> new file mode 100644 >>>>> index 0000000..84c8605 >>>>> --- /dev/null >>>>> +++ b/include/soc/qcom/qcom_minidump.h >>>>> @@ -0,0 +1,66 @@ >>>>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>>>> +/* >>>>> + * Qualcomm minidump shared data structures and macros >>>>> + * >>>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights >>>>> reserved. >>>>> + */ >>>>> + >>>>> +#ifndef _QCOM_MINIDUMP_H_ >>>>> +#define _QCOM_MINIDUMP_H_ >>>>> + >>>>> +#define MAX_NUM_OF_SS 10 >>>>> +#define MAX_REGION_NAME_LENGTH 16 >>>>> +#define SBL_MINIDUMP_SMEM_ID 602 >>>>> +#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' >>>>> << 8 | 'I' << 0) >>>>> +#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' >>>>> << 8 | 'E' << 0) >>>>> +#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << >>>>> 8 | 'L' << 0) >>>>> + >>>>> +/** >>>>> + * struct minidump_region - Minidump region >>>>> + * @name : Name of the region to be dumped >>>>> + * @seq_num: : Use to differentiate regions with same name. >>>>> + * @valid : This entry to be dumped (if set to 1) >>>>> + * @address : Physical address of region to be dumped >>>>> + * @size : Size of the region >>>>> + */ >>>>> +struct minidump_region { >>>>> + char name[MAX_REGION_NAME_LENGTH]; >>>>> + __le32 seq_num; >>>>> + __le32 valid; >>>>> + __le64 address; >>>>> + __le64 size; >>>>> +}; >>>>> + >>>>> +/** >>>>> + * struct minidump_subsystem - Subsystem's SMEM Table of content >>>>> + * @status : Subsystem toc init status >>>>> + * @enabled : if set to 1, this region would be copied during >>>>> coredump >>>>> + * @encryption_status: Encryption status for this subsystem >>>>> + * @encryption_required : Decides to encrypt the subsystem regions >>>>> or not >>>>> + * @region_count : Number of regions added in this subsystem toc >>>>> + * @regions_baseptr : regions base pointer of the subsystem >>>>> + */ >>>>> +struct minidump_subsystem { >>>>> + __le32 status; >>>>> + __le32 enabled; >>>>> + __le32 encryption_status; >>>>> + __le32 encryption_required; >>>>> + __le32 region_count; >>>>> + __le64 regions_baseptr; >>>>> +}; >>>>> + >>>>> +/** >>>>> + * struct minidump_global_toc - Global Table of Content >>>>> + * @status : Global Minidump init status >>>>> + * @md_revision : Minidump revision >>>>> + * @enabled : Minidump enable status >>>>> + * @subsystems : Array of subsystems toc >>>>> + */ >>>>> +struct minidump_global_toc { >>>>> + __le32 status; >>>>> + __le32 md_revision; >>>>> + __le32 enabled; >>>>> + struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; >>>>> +}; >>>>> + >>>>> +#endif /* _QCOM_MINIDUMP_H_ */
On 4/18/2023 7:32 PM, Srinivas Kandagatla wrote: > > > On 17/04/2023 14:22, Mukesh Ojha wrote: >> >> >> On 4/14/2023 4:10 PM, Srinivas Kandagatla wrote: >>> >>> >>> On 14/04/2023 08:05, Mukesh Ojha wrote: >>>> Thanks again for coming back on this. >>>> >>>> On 4/14/2023 4:02 AM, Srinivas Kandagatla wrote: >>>>> >>>>> >>>>> On 22/03/2023 13:30, Mukesh Ojha wrote: >>>>>> Move minidump specific data types and macros to a separate internal >>>>>> header(qcom_minidump.h) so that it can be shared among different >>>>> >>>>> minidump.h should be good as we are already in include/soc/qcom/ >>>> >>>> >>>> Initially, i wanted to protect the content of qcom_minidump.h >>>> between qcom_minidump.c and qcom_common.c >>>> >>>> Ideally, here qcom_minidump.h should be supplier/provider header and >>>> can >>> >>> Am not sure if I understand the supplier concept correctly. >>> AFAIU, we have a 2 sets of apis >>> >>> 1. get hold of minidump descriptor based on minidump_id fro gtoc >>> using qcom_minidump_subsystem_desc(). Am assuming which ever driver >>> uses this api will set there segments and regions in there respective >>> drivers. >>> >>> 2. setting regions/segments in APSS minidump descriptors which are >>> done via qcom_minidump_region_register(). TBH this should be renamed >>> to qcom_apss_minidump_region_register(). >>> >>> mixing of thsee apis makes it bit confusing, specially we have these >>> two category of apis that deal with different things. >>> >>> Does it make sense to spit and abstract them properly by doing? >>> >>> >>> 1. minidump driver to deal with handling gtoc and providing >>> descriptors to the consumers like remoteproc or apss, This can >>> probably live within smem driver as loc for this support is very >>> minimal and proabably rename the api accordingly. >>> >> >>> 2. apss_minidump driver to allow other qcom drivers to >>> register/unregister regions within apss minidump descriptor. >>> >>> did I miss something? >> >> No, you are correct with your understanding. >> >> To your suggestion to split code and to keep >> qcom_minidump_subsystem_desc() live inside smem driver, >> >> And how about the header qcom_minidump.h, should we keep it separate >> than the apss minidump client header minidump.h ? > > I tried to do what I suggested and it kinda looked okay w.r.t design but > started to look like a bit overdo. > > other thing that I tried like what you have in patches but creating a > dedicated platform driver for minidump (along with apss minidump), just > like other smem devices like socinfo.. > > This should also get rid of qcom_minidump_ready(). This is exactly i have said below . > >> >> Since, you kind of understand the driver now, do you think it is worth >> to create platform device for minidump from smem driver, and >> have a probe inside apss minidump driver to solve chicken and egg >> problem for the clients who comes before minidump and try to register >> itself without doing ready check and apss_minidump can note this >> client entry and only register this region once minidump probe success >> and then it can register all the noted clients in one go. > yes, it is doable. > >> >> The reason to do this would be apss_minidump driver has no meaning >> without smem, and for this reason checking qcom_minidump_ready() would >> not be ideal for at least qcom clients and for core kernel may be. >> > > how about something like this: We are totally in sync, Thanks for your time spent on the snippet. -Mukesh > > -------------------->cut<----------------------- > diff --git a/drivers/soc/qcom/minidump.c b/drivers/soc/qcom/minidump.c > new file mode 100644 > index 000000000000..4171054b268d > --- /dev/null > +++ b/drivers/soc/qcom/minidump.c > @@ -0,0 +1,100 @@ > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/soc/qcom/smem.h> > +#include <linux/soc/qcom/minidump.h> > + > +... > + > +struct minidump { > + struct minidump_global_toc *md_gbl_toc; > + struct minidump_subsystem *md_apss_toc; > + ... > +}; > + > +struct minidump *__md; > + > +struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int > minidump_index) > +{ > + if (!__md) > + return PTR_ERR(-EPROBE_DEFER); > + > + return &__mdgtoc->subsystems[minidump_index]; > +} > +EXPORT_SYMBOL(qcom_minidump_subsystem_desc); > + > + > +int qcom_minidump_apss_region_register(const struct > qcom_minidump_region *region) > +{ > + if (!__mdgtoc) > + return -EPROBE_DEFER; > + ... > +} > +EXPORT_SYMBOL_GPL(qcom_minidump_apss_region_register); > + > +static int qcom_minidump_init_apss_subsystem(struct minidumd *md) > +{ > + struct minidump_subsystem *apsstoc; > + > + apsstoc = qcom_minidump_subsystem_desc(MINIDUMP_APSS_DESC); > + if (IS_ERR(apsstoc)) > + return -EINVAL; > + > + md->md_regions = kcalloc(MAX_NUM_ENTRIES, > + sizeof(struct minidump_region), GFP_KERNEL); > + if (!md->md_regions) > + return -ENOMEM; > + > + minidump.apss_toc = apsstoc; > + > + ... > + > + return 0; > +} > +static int qcom_minidump_probe(struct platform_device *pdev) > +{ > + struct minidump *md; > + struct minidump_global_toc *mdgtoc; > + > + md = devm_kzalloc(&pdev->dev, sizeof(*md), GFP_KERNEL); > + > + ... > + mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, > &size); > + if (IS_ERR(mdgtoc)) { > + dev_err(&pdev->dev, "Couldn't find minidump\n"); > + return ERR_PTR(mdgtoc); > + } > + > + if (size < sizeof(*mdgtoc) || !mdgtoc->status) { > + dev_err(&pdev->dev, "Minidump table is not initialized\n"); > + return ERR_PTR(-EINVAL); > + } > + ... > + qcom_minidump_init_apss_subsystem(md); > + > + __md = md; > + ... > + return 0; > +} > + > +static int qcom_minidump_remove(struct platform_device *pdev) > +{ > + struct qcom_minidump *qs = platform_get_drvdata(pdev); > + > + ... > + > + return 0; > +} > + > +static struct platform_driver qcom_minidump_driver = { > + .probe = qcom_minidump_probe, > + .remove = qcom_minidump_remove, > + .driver = { > + .name = "qcom-minidump", > + }, > +}; > + > +module_platform_driver(qcom_minidump_driver); > + > +MODULE_DESCRIPTION("Qualcomm minidump driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:qcom-minidump"); > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c > index 4f163d62942c..d54b07095ddd 100644 > --- a/drivers/soc/qcom/smem.c > +++ b/drivers/soc/qcom/smem.c > @@ -279,6 +279,7 @@ struct qcom_smem { > > u32 item_count; > struct platform_device *socinfo; > + struct platform_device *minidump; > struct smem_ptable *ptable; > struct smem_partition global_partition; > struct smem_partition partitions[SMEM_HOST_COUNT]; > @@ -1151,12 +1152,19 @@ static int qcom_smem_probe(struct > platform_device *pdev) > if (IS_ERR(smem->socinfo)) > dev_dbg(&pdev->dev, "failed to register socinfo > device\n"); > > + smem->minidump = platform_device_register_data(&pdev->dev, > "qcom-minidump", > + PLATFORM_DEVID_NONE, NULL, > + 0); > + if (IS_ERR(smem->minidump)) > + dev_dbg(&pdev->dev, "failed to register minidump > device\n"); > + > return 0; > } > > static int qcom_smem_remove(struct platform_device *pdev) > { > platform_device_unregister(__smem->socinfo); > + platform_device_unregister(__smem->minidump); > > hwspin_lock_free(__smem->hwlock); > __smem = NULL; > (END) > -------------------->cut<----------------------- > >> --Mukesh >>> >>> thanks, >>> Srini >>> >>>> be shared among above qcom_minidump.c and qcom_common.c but since >>>> they are not in same directory, moved it inside include/soc/qcom/ as >>>> separate header than consumer header minidump.h . > >>>> -Mukesh >>>>> >>>>> --srini >>>>> >>>>>> Qualcomm drivers. >>>>>> >>>>>> There is no change in functional behavior after this. >>>>>> >>>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>>>>> --- >>>>>> drivers/remoteproc/qcom_common.c | 56 >>>>>> +--------------------------------- >>>>>> include/soc/qcom/qcom_minidump.h | 66 >>>>>> ++++++++++++++++++++++++++++++++++++++++ >>>>>> 2 files changed, 67 insertions(+), 55 deletions(-) >>>>>> create mode 100644 include/soc/qcom/qcom_minidump.h >>>>>> >>>>>> diff --git a/drivers/remoteproc/qcom_common.c >>>>>> b/drivers/remoteproc/qcom_common.c >>>>>> index 805e525..88fc984 100644 >>>>>> --- a/drivers/remoteproc/qcom_common.c >>>>>> +++ b/drivers/remoteproc/qcom_common.c >>>>>> @@ -18,6 +18,7 @@ >>>>>> #include <linux/slab.h> >>>>>> #include <linux/soc/qcom/mdt_loader.h> >>>>>> #include <linux/soc/qcom/smem.h> >>>>>> +#include <soc/qcom/qcom_minidump.h> >>>>>> #include "remoteproc_internal.h" >>>>>> #include "qcom_common.h" >>>>>> @@ -26,61 +27,6 @@ >>>>>> #define to_smd_subdev(d) container_of(d, struct >>>>>> qcom_rproc_subdev, subdev) >>>>>> #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, >>>>>> subdev) >>>>>> -#define MAX_NUM_OF_SS 10 >>>>>> -#define MAX_REGION_NAME_LENGTH 16 >>>>>> -#define SBL_MINIDUMP_SMEM_ID 602 >>>>>> -#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' >>>>>> << 8 | 'I' << 0) >>>>>> -#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' >>>>>> << 8 | 'E' << 0) >>>>>> -#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' >>>>>> << 8 | 'L' << 0) >>>>>> - >>>>>> -/** >>>>>> - * struct minidump_region - Minidump region >>>>>> - * @name : Name of the region to be dumped >>>>>> - * @seq_num: : Use to differentiate regions with same name. >>>>>> - * @valid : This entry to be dumped (if set to 1) >>>>>> - * @address : Physical address of region to be dumped >>>>>> - * @size : Size of the region >>>>>> - */ >>>>>> -struct minidump_region { >>>>>> - char name[MAX_REGION_NAME_LENGTH]; >>>>>> - __le32 seq_num; >>>>>> - __le32 valid; >>>>>> - __le64 address; >>>>>> - __le64 size; >>>>>> -}; >>>>>> - >>>>>> -/** >>>>>> - * struct minidump_subsystem - Subsystem's SMEM Table of content >>>>>> - * @status : Subsystem toc init status >>>>>> - * @enabled : if set to 1, this region would be copied during >>>>>> coredump >>>>>> - * @encryption_status: Encryption status for this subsystem >>>>>> - * @encryption_required : Decides to encrypt the subsystem >>>>>> regions or not >>>>>> - * @region_count : Number of regions added in this subsystem toc >>>>>> - * @regions_baseptr : regions base pointer of the subsystem >>>>>> - */ >>>>>> -struct minidump_subsystem { >>>>>> - __le32 status; >>>>>> - __le32 enabled; >>>>>> - __le32 encryption_status; >>>>>> - __le32 encryption_required; >>>>>> - __le32 region_count; >>>>>> - __le64 regions_baseptr; >>>>>> -}; >>>>>> - >>>>>> -/** >>>>>> - * struct minidump_global_toc - Global Table of Content >>>>>> - * @status : Global Minidump init status >>>>>> - * @md_revision : Minidump revision >>>>>> - * @enabled : Minidump enable status >>>>>> - * @subsystems : Array of subsystems toc >>>>>> - */ >>>>>> -struct minidump_global_toc { >>>>>> - __le32 status; >>>>>> - __le32 md_revision; >>>>>> - __le32 enabled; >>>>>> - struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; >>>>>> -}; >>>>>> - >>>>>> struct qcom_ssr_subsystem { >>>>>> const char *name; >>>>>> struct srcu_notifier_head notifier_list; >>>>>> diff --git a/include/soc/qcom/qcom_minidump.h >>>>>> b/include/soc/qcom/qcom_minidump.h >>>>>> new file mode 100644 >>>>>> index 0000000..84c8605 >>>>>> --- /dev/null >>>>>> +++ b/include/soc/qcom/qcom_minidump.h >>>>>> @@ -0,0 +1,66 @@ >>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>>>>> +/* >>>>>> + * Qualcomm minidump shared data structures and macros >>>>>> + * >>>>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights >>>>>> reserved. >>>>>> + */ >>>>>> + >>>>>> +#ifndef _QCOM_MINIDUMP_H_ >>>>>> +#define _QCOM_MINIDUMP_H_ >>>>>> + >>>>>> +#define MAX_NUM_OF_SS 10 >>>>>> +#define MAX_REGION_NAME_LENGTH 16 >>>>>> +#define SBL_MINIDUMP_SMEM_ID 602 >>>>>> +#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' >>>>>> << 8 | 'I' << 0) >>>>>> +#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' >>>>>> << 8 | 'E' << 0) >>>>>> +#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' >>>>>> << 8 | 'L' << 0) >>>>>> + >>>>>> +/** >>>>>> + * struct minidump_region - Minidump region >>>>>> + * @name : Name of the region to be dumped >>>>>> + * @seq_num: : Use to differentiate regions with same name. >>>>>> + * @valid : This entry to be dumped (if set to 1) >>>>>> + * @address : Physical address of region to be dumped >>>>>> + * @size : Size of the region >>>>>> + */ >>>>>> +struct minidump_region { >>>>>> + char name[MAX_REGION_NAME_LENGTH]; >>>>>> + __le32 seq_num; >>>>>> + __le32 valid; >>>>>> + __le64 address; >>>>>> + __le64 size; >>>>>> +}; >>>>>> + >>>>>> +/** >>>>>> + * struct minidump_subsystem - Subsystem's SMEM Table of content >>>>>> + * @status : Subsystem toc init status >>>>>> + * @enabled : if set to 1, this region would be copied during >>>>>> coredump >>>>>> + * @encryption_status: Encryption status for this subsystem >>>>>> + * @encryption_required : Decides to encrypt the subsystem >>>>>> regions or not >>>>>> + * @region_count : Number of regions added in this subsystem toc >>>>>> + * @regions_baseptr : regions base pointer of the subsystem >>>>>> + */ >>>>>> +struct minidump_subsystem { >>>>>> + __le32 status; >>>>>> + __le32 enabled; >>>>>> + __le32 encryption_status; >>>>>> + __le32 encryption_required; >>>>>> + __le32 region_count; >>>>>> + __le64 regions_baseptr; >>>>>> +}; >>>>>> + >>>>>> +/** >>>>>> + * struct minidump_global_toc - Global Table of Content >>>>>> + * @status : Global Minidump init status >>>>>> + * @md_revision : Minidump revision >>>>>> + * @enabled : Minidump enable status >>>>>> + * @subsystems : Array of subsystems toc >>>>>> + */ >>>>>> +struct minidump_global_toc { >>>>>> + __le32 status; >>>>>> + __le32 md_revision; >>>>>> + __le32 enabled; >>>>>> + struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; >>>>>> +}; >>>>>> + >>>>>> +#endif /* _QCOM_MINIDUMP_H_ */
diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c index 805e525..88fc984 100644 --- a/drivers/remoteproc/qcom_common.c +++ b/drivers/remoteproc/qcom_common.c @@ -18,6 +18,7 @@ #include <linux/slab.h> #include <linux/soc/qcom/mdt_loader.h> #include <linux/soc/qcom/smem.h> +#include <soc/qcom/qcom_minidump.h> #include "remoteproc_internal.h" #include "qcom_common.h" @@ -26,61 +27,6 @@ #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev) #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev) -#define MAX_NUM_OF_SS 10 -#define MAX_REGION_NAME_LENGTH 16 -#define SBL_MINIDUMP_SMEM_ID 602 -#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0) -#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0) -#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0) - -/** - * struct minidump_region - Minidump region - * @name : Name of the region to be dumped - * @seq_num: : Use to differentiate regions with same name. - * @valid : This entry to be dumped (if set to 1) - * @address : Physical address of region to be dumped - * @size : Size of the region - */ -struct minidump_region { - char name[MAX_REGION_NAME_LENGTH]; - __le32 seq_num; - __le32 valid; - __le64 address; - __le64 size; -}; - -/** - * struct minidump_subsystem - Subsystem's SMEM Table of content - * @status : Subsystem toc init status - * @enabled : if set to 1, this region would be copied during coredump - * @encryption_status: Encryption status for this subsystem - * @encryption_required : Decides to encrypt the subsystem regions or not - * @region_count : Number of regions added in this subsystem toc - * @regions_baseptr : regions base pointer of the subsystem - */ -struct minidump_subsystem { - __le32 status; - __le32 enabled; - __le32 encryption_status; - __le32 encryption_required; - __le32 region_count; - __le64 regions_baseptr; -}; - -/** - * struct minidump_global_toc - Global Table of Content - * @status : Global Minidump init status - * @md_revision : Minidump revision - * @enabled : Minidump enable status - * @subsystems : Array of subsystems toc - */ -struct minidump_global_toc { - __le32 status; - __le32 md_revision; - __le32 enabled; - struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; -}; - struct qcom_ssr_subsystem { const char *name; struct srcu_notifier_head notifier_list; diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h new file mode 100644 index 0000000..84c8605 --- /dev/null +++ b/include/soc/qcom/qcom_minidump.h @@ -0,0 +1,66 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Qualcomm minidump shared data structures and macros + * + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#ifndef _QCOM_MINIDUMP_H_ +#define _QCOM_MINIDUMP_H_ + +#define MAX_NUM_OF_SS 10 +#define MAX_REGION_NAME_LENGTH 16 +#define SBL_MINIDUMP_SMEM_ID 602 +#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0) +#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0) +#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0) + +/** + * struct minidump_region - Minidump region + * @name : Name of the region to be dumped + * @seq_num: : Use to differentiate regions with same name. + * @valid : This entry to be dumped (if set to 1) + * @address : Physical address of region to be dumped + * @size : Size of the region + */ +struct minidump_region { + char name[MAX_REGION_NAME_LENGTH]; + __le32 seq_num; + __le32 valid; + __le64 address; + __le64 size; +}; + +/** + * struct minidump_subsystem - Subsystem's SMEM Table of content + * @status : Subsystem toc init status + * @enabled : if set to 1, this region would be copied during coredump + * @encryption_status: Encryption status for this subsystem + * @encryption_required : Decides to encrypt the subsystem regions or not + * @region_count : Number of regions added in this subsystem toc + * @regions_baseptr : regions base pointer of the subsystem + */ +struct minidump_subsystem { + __le32 status; + __le32 enabled; + __le32 encryption_status; + __le32 encryption_required; + __le32 region_count; + __le64 regions_baseptr; +}; + +/** + * struct minidump_global_toc - Global Table of Content + * @status : Global Minidump init status + * @md_revision : Minidump revision + * @enabled : Minidump enable status + * @subsystems : Array of subsystems toc + */ +struct minidump_global_toc { + __le32 status; + __le32 md_revision; + __le32 enabled; + struct minidump_subsystem subsystems[MAX_NUM_OF_SS]; +}; + +#endif /* _QCOM_MINIDUMP_H_ */
Move minidump specific data types and macros to a separate internal header(qcom_minidump.h) so that it can be shared among different Qualcomm drivers. There is no change in functional behavior after this. Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> --- drivers/remoteproc/qcom_common.c | 56 +--------------------------------- include/soc/qcom/qcom_minidump.h | 66 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 55 deletions(-) create mode 100644 include/soc/qcom/qcom_minidump.h