linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Rob Herring <robh@kernel.org>
Cc: Imran Khan <kimran@codeaurora.org>,
	Stephen Boyd <sboyd@codeaurora.org>,
	Andy Gross <agross@codeaurora.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"open list:ARM/QUALCOMM SUPPORT" <linux-soc@vger.kernel.org>
Subject: Re: [v10, 2/2] Documentation/ABI: Add ABI information for QCOM socinfo driver
Date: Tue, 25 Apr 2017 16:08:37 -0700	[thread overview]
Message-ID: <20170425230837.GO15143@minitux> (raw)
In-Reply-To: <CAL_Jsq+qiSvD3ozvaVu+BbZi5gW8oQG3LDJfFggG2UnYgfbQYA@mail.gmail.com>

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

  reply	other threads:[~2017-04-25 23:08 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-20 16:47 [PATCH v10 0/2] soc: qcom: Add SoC info driver Imran Khan
2017-02-20 16:47 ` [PATCH v10 1/2] " Imran Khan
2017-02-20 16:47 ` [PATCH v10 2/2] Documentation/ABI: Add ABI information for QCOM socinfo driver Imran Khan
2017-02-22 14:04   ` [v10, " Rob Herring
2017-03-06  6:49     ` Imran Khan
2017-04-18 14:23       ` Imran Khan
2017-04-24 16:00         ` Imran Khan
2017-04-24 16:27           ` Rob Herring
2017-04-24 23:05             ` Bjorn Andersson
2017-04-25  7:35               ` Imran Khan
2017-04-25 17:13               ` Rob Herring
2017-04-25 23:08                 ` Bjorn Andersson [this message]
2017-05-01 13:07                   ` Imran Khan
2017-05-01 22:47                     ` Bjorn Andersson
2017-05-03 12:51                       ` Imran Khan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170425230837.GO15143@minitux \
    --to=bjorn.andersson@linaro.org \
    --cc=agross@codeaurora.org \
    --cc=kimran@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=robh@kernel.org \
    --cc=sboyd@codeaurora.org \
    --subject='Re: [v10, 2/2] Documentation/ABI: Add ABI information for QCOM socinfo driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).