Re: [v10, 2/2] Documentation/ABI: Add ABI information for QCOM socinfo driver

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Tue 25 Apr 10:13 PDT 2017, Rob Herring wrote:

> On Mon, Apr 24, 2017 at 6:05 PM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Mon 24 Apr 09:27 PDT 2017, Rob Herring wrote:
[..]
> >> Splitting things between common and private seems like a good direction.
> >>
> >
> > But the pattern of amending the common attributes with vendor or
> > platform-specific ones is how its done in other drivers, e.g.
> > integrator.
> >
> > Why should we for the Qualcomm case make up some other pattern?
> >
> > Or am I misunderstanding your suggestion?
> 
> I'm just trying to ensure that we make things that could be common,
> common. The question here was about the implementation.
> 
> Right now, drivers/soc is just a free for all dumping ground IMO.
> 

Unfortunately I fully agree with this, we did spend several revisions on
just trying to match Qualcomm properties to the common attributes - and
I don't know if we got it right.

> >> >>>>> diff --git a/Documentation/ABI/testing/sysfs-driver-qcom_socinfo b/Documentation/ABI/testing/sysfs-driver-qcom_socinfo
> >> >>>>> new file mode 100644
> >> >>>>> index 0000000..cce611f
> >> >>>>> --- /dev/null
> >> >>>>> +++ b/Documentation/ABI/testing/sysfs-driver-qcom_socinfo
> >> >>>>> @@ -0,0 +1,171 @@
> >> >>>>> +What:             /sys/devices/soc0/accessory_chip
> >> >>>>> +Date:             January 2017
> >> >>>>> +Contact:  linux-arm-msm@vger.kernel.org
> >> >>>>> +Description:
> >> >>>>> +          This file shows the id of the accessory chip.
> >> >>>>> +
> >> >>>>> +What:             /sys/devices/soc0/adsp_image_crm
> >> >>>>> +What:             /sys/devices/soc0/adsp_image_variant
> >> >>>>> +What:             /sys/devices/soc0/adsp_image_version
> >> >>>>> +Date:             January 2017
> >> >>>>> +Contact:  linux-arm-msm@vger.kernel.org
> >> >>>>> +Description:
> >> >>>>> +          These files respectively show the crm version, variant and
> >> >>>>> +          version of the ADSP image.
> >> >>>>
> >> >>>> Shouldn't this be part of the ADSP driver?
> >> >>>>
> >> >>> Yes, It can be but I wanted to keep the image information at a central location,
> >> >>> rather than pushing it back to each driver. For image information we basically
> >> >>> read the same item from SMEM but use different offsets within it for different images,
> >> >>> so the idea was to read this information ( get SMEM handler) just once, rather than
> >> >>> doing it for each driver.
> >> >>> But if this idea does not look correct, I can remove it from socinfo driver.
> >> >>>
> >> >>
> >> >> Could you please provide some feedback regarding this?
> >>
> >> I don't think parsing things once will save you much.
> >>
> >
> > Defining the struct in some shared header file and implementing the
> > "parsing" throughout various drivers would probably work out fine.
> >
> > What I do not fancy is where these "parsers" would be implemented and
> > where the information would end up in sysfs.
> >
> > "The ADSP driver" has to refer to the remoteproc driver for the adsp and
> > it would be reasonable to have version information of the firmware
> > available for a running piece of remoteproc. The same would work out for
> > modem and wireless.
> >
> > The v4l driver is responsible for controlling the venus core, so it
> > could provide the version attribute - which would show up somewhere in
> > the /sys/bus/platform hierarchy.
> >
> > We have a mfd-like driver for communicating with the RPM, so perhaps we
> > could shoehorn in an attribute there. But the sysfs path will be
> > completely different, depending on platform and system configuration.
> >
> > There is no driver representing the boot code.
> >
> >
> > So, when Android goes belly up and we want to produce a bugreport that
> > describes all the pieces of the system we will have to all over sysfs to
> > figure out the versions of the firmware...
> 
> Can't you just use the meta build id? That implies the version of
> *everything*, right?
> 

For products yes, but when _you_ ask us why WiFi doesn't work on your
Dragonboard it's quite nice to be able to get hold of the boot-loader
and wcnss-firmware versions - and you're likely not on an unmodified
meta-build.

For me this information is more valuable than the meta-build id, but
that's because I'm working with engineering builds.

[..]
> >> >>>>> +What:             /sys/devices/soc0/build_id
> >> >>>>> +Date:             January 2017
> >> >>>>> +Contact:  linux-arm-msm@vger.kernel.org
> >> >>>>> +Description:
> >> >>>>> +          This file shows the unique id of current build being used on
> >> >>>>> +          the system.
> >> >>>>
> >> >>>> Build of what? The kernel already has a build version.
> >> >>>>
> >> >>> This is not build id of the kernel. This is build ID of complete meta image.
> >>
> >> That's assuming everything is built together which generally isn't
> >> true.
> >
> > Not necessarily compiled together, but packaged up in something that
> > gets a "release number" - which I presume is quite common for any type
> > of embedded products.
> >
> > E.g. we do give the Linaro releases version numbers such as "16.09".
> 
> Yes, but I doubt the release number is visible inside the release
> unless it is part of some existing version like the kernel's version
> string. If this is quite common, then lets make it common.
> 

I don't know how common this is - as you suggest below perhaps people
just put it in one of the file systems?

> Why not just make this a file in the filesystem?

Because that forces you to rebuild your file system to update the
version of the system. That said I don't know the details of how
Qualcomm persistently encode this information.

> Why does the kernel need to provide it?
> 

Imran, can you elaborate on how this information is travels from the
build system to the SMEM item?

Regards,
Bjorn