Re: [PATCH v2 2/3] soc: qcom: socinfo: Add support for new fields in revision 21

[Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>]

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