| Message ID | 20250425135946.1087065-2-mukesh.ojha@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] soc: qcom: socinfo: Add support for new fields in revision 20 | expand |
On Fri, Apr 25, 2025 at 07:29:45PM +0530, Mukesh Ojha wrote: > Add the subpartfeature offset field to the socinfo structure > which came for version 21 of socinfo structure. > > Subpart_feat_offset is subpart like camera, display, etc., > and its internal feature available on a bin. > > Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com> > --- > Changes in v2: > - Added debugfs entry and described more about the field in commit. > > drivers/soc/qcom/socinfo.c | 6 ++++++ > include/linux/soc/qcom/socinfo.h | 2 ++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c > index 5800ebf9ceea..bac1485f1b27 100644 > --- a/drivers/soc/qcom/socinfo.c > +++ b/drivers/soc/qcom/socinfo.c > @@ -154,6 +154,7 @@ struct socinfo_params { > u32 boot_cluster; > u32 boot_core; > u32 raw_package_type; > + u32 nsubpart_feat_array_offset; > }; > > struct smem_image_version { > @@ -608,6 +609,11 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo, > &qcom_socinfo->info.fmt); > > switch (qcom_socinfo->info.fmt) { > + case SOCINFO_VERSION(0, 21): > + qcom_socinfo->info.nsubpart_feat_array_offset = > + __le32_to_cpu(info->nsubpart_feat_array_offset); > + debugfs_create_u32("nsubpart_feat_array_offset", 0444, qcom_socinfo->dbg_root, > + &qcom_socinfo->info.nsubpart_feat_array_offset); An offset into what? If this provides additional data, then the data should be visible in the debugfs. Not sure, what's the point in dumping the offset here. > case SOCINFO_VERSION(0, 20): > qcom_socinfo->info.raw_package_type = __le32_to_cpu(info->raw_package_type); > debugfs_create_u32("raw_package_type", 0444, qcom_socinfo->dbg_root, > diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h > index c4dae173cc30..3666870b7988 100644 > --- a/include/linux/soc/qcom/socinfo.h > +++ b/include/linux/soc/qcom/socinfo.h > @@ -84,6 +84,8 @@ struct socinfo { > __le32 boot_core; > /* Version 20 */ > __le32 raw_package_type; > + /* Version 21 */ > + __le32 nsubpart_feat_array_offset; > }; > > /* Internal feature codes */ > -- > 2.34.1 >
On Fri, Apr 25, 2025 at 08:28:51PM +0300, Dmitry Baryshkov wrote: > On Fri, Apr 25, 2025 at 07:29:45PM +0530, Mukesh Ojha wrote: > > Add the subpartfeature offset field to the socinfo structure > > which came for version 21 of socinfo structure. > > > > Subpart_feat_offset is subpart like camera, display, etc., > > and its internal feature available on a bin. > > > > Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com> > > --- > > Changes in v2: > > - Added debugfs entry and described more about the field in commit. > > > > drivers/soc/qcom/socinfo.c | 6 ++++++ > > include/linux/soc/qcom/socinfo.h | 2 ++ > > 2 files changed, 8 insertions(+) > > > > diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c > > index 5800ebf9ceea..bac1485f1b27 100644 > > --- a/drivers/soc/qcom/socinfo.c > > +++ b/drivers/soc/qcom/socinfo.c > > @@ -154,6 +154,7 @@ struct socinfo_params { > > u32 boot_cluster; > > u32 boot_core; > > u32 raw_package_type; > > + u32 nsubpart_feat_array_offset; > > }; > > > > struct smem_image_version { > > @@ -608,6 +609,11 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo, > > &qcom_socinfo->info.fmt); > > > > switch (qcom_socinfo->info.fmt) { > > + case SOCINFO_VERSION(0, 21): > > + qcom_socinfo->info.nsubpart_feat_array_offset = > > + __le32_to_cpu(info->nsubpart_feat_array_offset); > > + debugfs_create_u32("nsubpart_feat_array_offset", 0444, qcom_socinfo->dbg_root, > > + &qcom_socinfo->info.nsubpart_feat_array_offset); > > An offset into what? If this provides additional data, then the data > should be visible in the debugfs. Not sure, what's the point in dumping > the offset here. offset into info(struct socinfo) object. I agree to you and I said the same in first version this is just offset and does not provide any debug info we would look from userspace. For parity with other fields I did it for all newly added fields. I have dropped it in latest patch. -Mukesh
On Thu, May 08, 2025 at 09:07:03PM +0530, Mukesh Ojha wrote: > On Fri, Apr 25, 2025 at 08:28:51PM +0300, Dmitry Baryshkov wrote: > > On Fri, Apr 25, 2025 at 07:29:45PM +0530, Mukesh Ojha wrote: > > > Add the subpartfeature offset field to the socinfo structure > > > which came for version 21 of socinfo structure. > > > > > > Subpart_feat_offset is subpart like camera, display, etc., > > > and its internal feature available on a bin. > > > > > > Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com> > > > --- > > > Changes in v2: > > > - Added debugfs entry and described more about the field in commit. > > > > > > drivers/soc/qcom/socinfo.c | 6 ++++++ > > > include/linux/soc/qcom/socinfo.h | 2 ++ > > > 2 files changed, 8 insertions(+) > > > > > > diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c > > > index 5800ebf9ceea..bac1485f1b27 100644 > > > --- a/drivers/soc/qcom/socinfo.c > > > +++ b/drivers/soc/qcom/socinfo.c > > > @@ -154,6 +154,7 @@ struct socinfo_params { > > > u32 boot_cluster; > > > u32 boot_core; > > > u32 raw_package_type; > > > + u32 nsubpart_feat_array_offset; > > > }; > > > > > > struct smem_image_version { > > > @@ -608,6 +609,11 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo, > > > &qcom_socinfo->info.fmt); > > > > > > switch (qcom_socinfo->info.fmt) { > > > + case SOCINFO_VERSION(0, 21): > > > + qcom_socinfo->info.nsubpart_feat_array_offset = > > > + __le32_to_cpu(info->nsubpart_feat_array_offset); > > > + debugfs_create_u32("nsubpart_feat_array_offset", 0444, qcom_socinfo->dbg_root, > > > + &qcom_socinfo->info.nsubpart_feat_array_offset); > > > > An offset into what? If this provides additional data, then the data > > should be visible in the debugfs. Not sure, what's the point in dumping > > the offset here. > > offset into info(struct socinfo) object. > > I agree to you and I said the same in first version this is just offset > and does not provide any debug info we would look from userspace. For > parity with other fields I did it for all newly added fields. > I have dropped it in latest patch. I'd rather see the decoded structure that is being pointed by this offset.
On Thu, May 08, 2025 at 06:56:47PM +0300, Dmitry Baryshkov wrote: > On Thu, May 08, 2025 at 09:07:03PM +0530, Mukesh Ojha wrote: > > On Fri, Apr 25, 2025 at 08:28:51PM +0300, Dmitry Baryshkov wrote: > > > On Fri, Apr 25, 2025 at 07:29:45PM +0530, Mukesh Ojha wrote: > > > > Add the subpartfeature offset field to the socinfo structure > > > > which came for version 21 of socinfo structure. > > > > > > > > Subpart_feat_offset is subpart like camera, display, etc., > > > > and its internal feature available on a bin. > > > > > > > > Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com> > > > > --- > > > > Changes in v2: > > > > - Added debugfs entry and described more about the field in commit. > > > > > > > > drivers/soc/qcom/socinfo.c | 6 ++++++ > > > > include/linux/soc/qcom/socinfo.h | 2 ++ > > > > 2 files changed, 8 insertions(+) > > > > > > > > diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c > > > > index 5800ebf9ceea..bac1485f1b27 100644 > > > > --- a/drivers/soc/qcom/socinfo.c > > > > +++ b/drivers/soc/qcom/socinfo.c > > > > @@ -154,6 +154,7 @@ struct socinfo_params { > > > > u32 boot_cluster; > > > > u32 boot_core; > > > > u32 raw_package_type; > > > > + u32 nsubpart_feat_array_offset; > > > > }; > > > > > > > > struct smem_image_version { > > > > @@ -608,6 +609,11 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo, > > > > &qcom_socinfo->info.fmt); > > > > > > > > switch (qcom_socinfo->info.fmt) { > > > > + case SOCINFO_VERSION(0, 21): > > > > + qcom_socinfo->info.nsubpart_feat_array_offset = > > > > + __le32_to_cpu(info->nsubpart_feat_array_offset); > > > > + debugfs_create_u32("nsubpart_feat_array_offset", 0444, qcom_socinfo->dbg_root, > > > > + &qcom_socinfo->info.nsubpart_feat_array_offset); > > > > > > An offset into what? If this provides additional data, then the data > > > should be visible in the debugfs. Not sure, what's the point in dumping > > > the offset here. > > > > offset into info(struct socinfo) object. > > > > I agree to you and I said the same in first version this is just offset > > and does not provide any debug info we would look from userspace. For > > parity with other fields I did it for all newly added fields. > > I have dropped it in latest patch. > > I'd rather see the decoded structure that is being pointed by this > offset. You mean info + info->nsubpart_feat_array_offset ? There is more to it which I don't want to mention as they are not upstreamed yet and unrelated to this change. data = info + (offset + (part * sizeof(u32))); e.g., Here, part is a enum represents camera, display etc., and data represents their feature presents. Since, part is not upstream yet I don't feel we should expose this information to debugfs. We could always add them in debugfs when such things are standardized and upstreamed. -Mukesh > > -- > With best wishes > Dmitry
On 5/8/25 6:48 PM, Mukesh Ojha wrote: > On Thu, May 08, 2025 at 06:56:47PM +0300, Dmitry Baryshkov wrote: >> On Thu, May 08, 2025 at 09:07:03PM +0530, Mukesh Ojha wrote: >>> On Fri, Apr 25, 2025 at 08:28:51PM +0300, Dmitry Baryshkov wrote: >>>> On Fri, Apr 25, 2025 at 07:29:45PM +0530, Mukesh Ojha wrote: >>>>> Add the subpartfeature offset field to the socinfo structure >>>>> which came for version 21 of socinfo structure. >>>>> >>>>> Subpart_feat_offset is subpart like camera, display, etc., >>>>> and its internal feature available on a bin. >>>>> >>>>> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com> >>>>> --- >>>>> Changes in v2: >>>>> - Added debugfs entry and described more about the field in commit. >>>>> >>>>> drivers/soc/qcom/socinfo.c | 6 ++++++ >>>>> include/linux/soc/qcom/socinfo.h | 2 ++ >>>>> 2 files changed, 8 insertions(+) >>>>> >>>>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c >>>>> index 5800ebf9ceea..bac1485f1b27 100644 >>>>> --- a/drivers/soc/qcom/socinfo.c >>>>> +++ b/drivers/soc/qcom/socinfo.c >>>>> @@ -154,6 +154,7 @@ struct socinfo_params { >>>>> u32 boot_cluster; >>>>> u32 boot_core; >>>>> u32 raw_package_type; >>>>> + u32 nsubpart_feat_array_offset; >>>>> }; >>>>> >>>>> struct smem_image_version { >>>>> @@ -608,6 +609,11 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo, >>>>> &qcom_socinfo->info.fmt); >>>>> >>>>> switch (qcom_socinfo->info.fmt) { >>>>> + case SOCINFO_VERSION(0, 21): >>>>> + qcom_socinfo->info.nsubpart_feat_array_offset = >>>>> + __le32_to_cpu(info->nsubpart_feat_array_offset); >>>>> + debugfs_create_u32("nsubpart_feat_array_offset", 0444, qcom_socinfo->dbg_root, >>>>> + &qcom_socinfo->info.nsubpart_feat_array_offset); >>>> >>>> An offset into what? If this provides additional data, then the data >>>> should be visible in the debugfs. Not sure, what's the point in dumping >>>> the offset here. >>> >>> offset into info(struct socinfo) object. >>> >>> I agree to you and I said the same in first version this is just offset >>> and does not provide any debug info we would look from userspace. For >>> parity with other fields I did it for all newly added fields. >>> I have dropped it in latest patch. >> >> I'd rather see the decoded structure that is being pointed by this >> offset. > > You mean info + info->nsubpart_feat_array_offset ? > > There is more to it which I don't want to mention as they are not > upstreamed yet and unrelated to this change. > > data = info + (offset + (part * sizeof(u32))); > > e.g., Here, part is a enum represents camera, display etc., and data > represents their feature presents. Since, part is not upstream yet I > don't feel we should expose this information to debugfs. We could always > add them in debugfs when such things are standardized and upstreamed. That's what Dmitry's saying - just add support for them Konrad
On Thu, May 08, 2025 at 08:01:44PM +0200, Konrad Dybcio wrote: > On 5/8/25 6:48 PM, Mukesh Ojha wrote: > > On Thu, May 08, 2025 at 06:56:47PM +0300, Dmitry Baryshkov wrote: > >> On Thu, May 08, 2025 at 09:07:03PM +0530, Mukesh Ojha wrote: > >>> On Fri, Apr 25, 2025 at 08:28:51PM +0300, Dmitry Baryshkov wrote: > >>>> On Fri, Apr 25, 2025 at 07:29:45PM +0530, Mukesh Ojha wrote: > >>>>> Add the subpartfeature offset field to the socinfo structure > >>>>> which came for version 21 of socinfo structure. > >>>>> > >>>>> Subpart_feat_offset is subpart like camera, display, etc., > >>>>> and its internal feature available on a bin. > >>>>> > >>>>> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com> > >>>>> --- > >>>>> Changes in v2: > >>>>> - Added debugfs entry and described more about the field in commit. > >>>>> > >>>>> drivers/soc/qcom/socinfo.c | 6 ++++++ > >>>>> include/linux/soc/qcom/socinfo.h | 2 ++ > >>>>> 2 files changed, 8 insertions(+) > >>>>> > >>>>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c > >>>>> index 5800ebf9ceea..bac1485f1b27 100644 > >>>>> --- a/drivers/soc/qcom/socinfo.c > >>>>> +++ b/drivers/soc/qcom/socinfo.c > >>>>> @@ -154,6 +154,7 @@ struct socinfo_params { > >>>>> u32 boot_cluster; > >>>>> u32 boot_core; > >>>>> u32 raw_package_type; > >>>>> + u32 nsubpart_feat_array_offset; > >>>>> }; > >>>>> > >>>>> struct smem_image_version { > >>>>> @@ -608,6 +609,11 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo, > >>>>> &qcom_socinfo->info.fmt); > >>>>> > >>>>> switch (qcom_socinfo->info.fmt) { > >>>>> + case SOCINFO_VERSION(0, 21): > >>>>> + qcom_socinfo->info.nsubpart_feat_array_offset = > >>>>> + __le32_to_cpu(info->nsubpart_feat_array_offset); > >>>>> + debugfs_create_u32("nsubpart_feat_array_offset", 0444, qcom_socinfo->dbg_root, > >>>>> + &qcom_socinfo->info.nsubpart_feat_array_offset); > >>>> > >>>> An offset into what? If this provides additional data, then the data > >>>> should be visible in the debugfs. Not sure, what's the point in dumping > >>>> the offset here. > >>> > >>> offset into info(struct socinfo) object. > >>> > >>> I agree to you and I said the same in first version this is just offset > >>> and does not provide any debug info we would look from userspace. For > >>> parity with other fields I did it for all newly added fields. > >>> I have dropped it in latest patch. > >> > >> I'd rather see the decoded structure that is being pointed by this > >> offset. > > > > You mean info + info->nsubpart_feat_array_offset ? > > > > There is more to it which I don't want to mention as they are not > > upstreamed yet and unrelated to this change. > > > > data = info + (offset + (part * sizeof(u32))); > > > > e.g., Here, part is a enum represents camera, display etc., and data > > represents their feature presents. Since, part is not upstream yet I > > don't feel we should expose this information to debugfs. We could always > > add them in debugfs when such things are standardized and upstreamed. > > That's what Dmitry's saying - just add support for them We definitely add support for this in the future. In the meantime, does the absence of the support prevent this socinfo field from being merged? Without it, there could be inconsistencies between the boot firmware and Linux for the SM8750 platform. -Mukesh > > Konrad
On 5/9/25 12:06 PM, Mukesh Ojha wrote: > On Thu, May 08, 2025 at 08:01:44PM +0200, Konrad Dybcio wrote: >> On 5/8/25 6:48 PM, Mukesh Ojha wrote: >>> On Thu, May 08, 2025 at 06:56:47PM +0300, Dmitry Baryshkov wrote: >>>> On Thu, May 08, 2025 at 09:07:03PM +0530, Mukesh Ojha wrote: >>>>> On Fri, Apr 25, 2025 at 08:28:51PM +0300, Dmitry Baryshkov wrote: >>>>>> On Fri, Apr 25, 2025 at 07:29:45PM +0530, Mukesh Ojha wrote: >>>>>>> Add the subpartfeature offset field to the socinfo structure >>>>>>> which came for version 21 of socinfo structure. >>>>>>> >>>>>>> Subpart_feat_offset is subpart like camera, display, etc., >>>>>>> and its internal feature available on a bin. >>>>>>> >>>>>>> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com> >>>>>>> --- >>>>>>> Changes in v2: >>>>>>> - Added debugfs entry and described more about the field in commit. >>>>>>> >>>>>>> drivers/soc/qcom/socinfo.c | 6 ++++++ >>>>>>> include/linux/soc/qcom/socinfo.h | 2 ++ >>>>>>> 2 files changed, 8 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c >>>>>>> index 5800ebf9ceea..bac1485f1b27 100644 >>>>>>> --- a/drivers/soc/qcom/socinfo.c >>>>>>> +++ b/drivers/soc/qcom/socinfo.c >>>>>>> @@ -154,6 +154,7 @@ struct socinfo_params { >>>>>>> u32 boot_cluster; >>>>>>> u32 boot_core; >>>>>>> u32 raw_package_type; >>>>>>> + u32 nsubpart_feat_array_offset; >>>>>>> }; >>>>>>> >>>>>>> struct smem_image_version { >>>>>>> @@ -608,6 +609,11 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo, >>>>>>> &qcom_socinfo->info.fmt); >>>>>>> >>>>>>> switch (qcom_socinfo->info.fmt) { >>>>>>> + case SOCINFO_VERSION(0, 21): >>>>>>> + qcom_socinfo->info.nsubpart_feat_array_offset = >>>>>>> + __le32_to_cpu(info->nsubpart_feat_array_offset); >>>>>>> + debugfs_create_u32("nsubpart_feat_array_offset", 0444, qcom_socinfo->dbg_root, >>>>>>> + &qcom_socinfo->info.nsubpart_feat_array_offset); >>>>>> >>>>>> An offset into what? If this provides additional data, then the data >>>>>> should be visible in the debugfs. Not sure, what's the point in dumping >>>>>> the offset here. >>>>> >>>>> offset into info(struct socinfo) object. >>>>> >>>>> I agree to you and I said the same in first version this is just offset >>>>> and does not provide any debug info we would look from userspace. For >>>>> parity with other fields I did it for all newly added fields. >>>>> I have dropped it in latest patch. >>>> >>>> I'd rather see the decoded structure that is being pointed by this >>>> offset. >>> >>> You mean info + info->nsubpart_feat_array_offset ? >>> >>> There is more to it which I don't want to mention as they are not >>> upstreamed yet and unrelated to this change. >>> >>> data = info + (offset + (part * sizeof(u32))); >>> >>> e.g., Here, part is a enum represents camera, display etc., and data >>> represents their feature presents. Since, part is not upstream yet I >>> don't feel we should expose this information to debugfs. We could always >>> add them in debugfs when such things are standardized and upstreamed. >> >> That's what Dmitry's saying - just add support for them > > We definitely add support for this in the future. In the meantime, does > the absence of the support prevent this socinfo field from being merged? > Without it, there could be inconsistencies between the boot firmware and > Linux for the SM8750 platform. Why delay adding a couple struct definitions into the "future"? I feel like we spent more time on this thread that describing them would take Konrad
diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c index 5800ebf9ceea..bac1485f1b27 100644 --- a/drivers/soc/qcom/socinfo.c +++ b/drivers/soc/qcom/socinfo.c @@ -154,6 +154,7 @@ struct socinfo_params { u32 boot_cluster; u32 boot_core; u32 raw_package_type; + u32 nsubpart_feat_array_offset; }; struct smem_image_version { @@ -608,6 +609,11 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo, &qcom_socinfo->info.fmt); switch (qcom_socinfo->info.fmt) { + case SOCINFO_VERSION(0, 21): + qcom_socinfo->info.nsubpart_feat_array_offset = + __le32_to_cpu(info->nsubpart_feat_array_offset); + debugfs_create_u32("nsubpart_feat_array_offset", 0444, qcom_socinfo->dbg_root, + &qcom_socinfo->info.nsubpart_feat_array_offset); case SOCINFO_VERSION(0, 20): qcom_socinfo->info.raw_package_type = __le32_to_cpu(info->raw_package_type); debugfs_create_u32("raw_package_type", 0444, qcom_socinfo->dbg_root, diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h index c4dae173cc30..3666870b7988 100644 --- a/include/linux/soc/qcom/socinfo.h +++ b/include/linux/soc/qcom/socinfo.h @@ -84,6 +84,8 @@ struct socinfo { __le32 boot_core; /* Version 20 */ __le32 raw_package_type; + /* Version 21 */ + __le32 nsubpart_feat_array_offset; }; /* Internal feature codes */
Add the subpartfeature offset field to the socinfo structure which came for version 21 of socinfo structure. Subpart_feat_offset is subpart like camera, display, etc., and its internal feature available on a bin. Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com> --- Changes in v2: - Added debugfs entry and described more about the field in commit. drivers/soc/qcom/socinfo.c | 6 ++++++ include/linux/soc/qcom/socinfo.h | 2 ++ 2 files changed, 8 insertions(+)