From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Suman Anna <s-anna@ti.com>
Cc: Rob Herring <robh+dt@kernel.org>,
Mathieu Poirier <mathieu.poirier@linaro.org>,
Clement Leger <cleger@kalray.eu>,
Loic Pallardy <loic.pallardy@st.com>,
Arnaud Pouliquen <arnaud.pouliquen@st.com>,
Lokesh Vutla <lokeshvutla@ti.com>,
linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] remoteproc: add support for a new 64-bit trace version
Date: Thu, 21 May 2020 11:04:17 -0700 [thread overview]
Message-ID: <20200521180417.GJ408178@builder.lan> (raw)
In-Reply-To: <20200325204701.16862-4-s-anna@ti.com>
On Wed 25 Mar 13:47 PDT 2020, Suman Anna wrote:
> Introduce a new trace entry resource structure that accommodates
> a 64-bit device address to support 64-bit processors. This is to
> be used using an overloaded version value of 1 in the upper 32-bits
> of the previous resource type field. The new resource still uses
> 32-bits for the length field (followed by a 32-bit reserved field,
> so can be updated in the future), which is a sufficiently large
> trace buffer size. A 32-bit padding field also had to be added
> to align the device address on a 64-bit boundary, and match the
> usage on the firmware side.
>
> The remoteproc debugfs logic also has been adjusted accordingly.
>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> drivers/remoteproc/remoteproc_core.c | 40 ++++++++++++++++++++-----
> drivers/remoteproc/remoteproc_debugfs.c | 37 ++++++++++++++++++-----
> include/linux/remoteproc.h | 26 ++++++++++++++++
> 3 files changed, 87 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 53bc37c508c6..b9a097990862 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -609,21 +609,45 @@ void rproc_vdev_release(struct kref *ref)
> *
> * Returns 0 on success, or an appropriate error code otherwise
> */
> -static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
> +static int rproc_handle_trace(struct rproc *rproc, void *rsc,
> int offset, int avail, u16 ver)
> {
> struct rproc_debug_trace *trace;
> struct device *dev = &rproc->dev;
> + struct fw_rsc_trace *rsc1;
> + struct fw_rsc_trace2 *rsc2;
> char name[15];
> + size_t rsc_size;
> + u32 reserved;
> + u64 da;
> + u32 len;
> +
> + if (!ver) {
This looks like a switch to me, but I also do think this looks rather
crude, if you spin off the tail of this function and call it from a
rproc_handle_trace() and rproc_handle_trace64() I believe this would be
cleaner.
> + rsc1 = (struct fw_rsc_trace *)rsc;
> + rsc_size = sizeof(*rsc1);
> + reserved = rsc1->reserved;
> + da = rsc1->da;
> + len = rsc1->len;
> + } else if (ver == 1) {
> + rsc2 = (struct fw_rsc_trace2 *)rsc;
> + rsc_size = sizeof(*rsc2);
> + reserved = rsc2->reserved;
> + da = rsc2->da;
> + len = rsc2->len;
> + } else {
> + dev_err(dev, "unsupported trace rsc version %d\n", ver);
If we use "type" to describe your 64-bit-da-trace then this sanity check
would have been taken care of by the core.
> + return -EINVAL;
> + }
>
> - if (sizeof(*rsc) > avail) {
> + if (rsc_size > avail) {
> dev_err(dev, "trace rsc is truncated\n");
> return -EINVAL;
> }
>
> /* make sure reserved bytes are zeroes */
> - if (rsc->reserved) {
> - dev_err(dev, "trace rsc has non zero reserved bytes\n");
> + if (reserved) {
> + dev_err(dev, "trace rsc has non zero reserved bytes, value = 0x%x\n",
> + reserved);
> return -EINVAL;
> }
>
> @@ -632,8 +656,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
> return -ENOMEM;
>
> /* set the trace buffer dma properties */
> - trace->trace_mem.len = rsc->len;
> - trace->trace_mem.da = rsc->da;
> + trace->trace_mem.len = len;
> + trace->trace_mem.da = da;
>
> /* set pointer on rproc device */
> trace->rproc = rproc;
> @@ -652,8 +676,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>
> rproc->num_traces++;
>
> - dev_dbg(dev, "%s added: da 0x%x, len 0x%x\n",
> - name, rsc->da, rsc->len);
> + dev_dbg(dev, "%s added: da 0x%llx, len 0x%x\n",
> + name, da, len);
>
> return 0;
> }
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index 3560eed7a360..ff43736db45a 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -192,7 +192,8 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
> struct resource_table *table = rproc->table_ptr;
> struct fw_rsc_carveout *c;
> struct fw_rsc_devmem *d;
> - struct fw_rsc_trace *t;
> + struct fw_rsc_trace *t1;
> + struct fw_rsc_trace2 *t2;
> struct fw_rsc_vdev *v;
> int i, j;
>
> @@ -205,6 +206,7 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
> int offset = table->offset[i];
> struct fw_rsc_hdr *hdr = (void *)table + offset;
> void *rsc = (void *)hdr + sizeof(*hdr);
> + u16 ver = hdr->st.v;
>
> switch (hdr->st.t) {
> case RSC_CARVEOUT:
> @@ -230,13 +232,32 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
> seq_printf(seq, " Name %s\n\n", d->name);
> break;
> case RSC_TRACE:
> - t = rsc;
> - seq_printf(seq, "Entry %d is of type %s\n",
> - i, types[hdr->st.t]);
> - seq_printf(seq, " Device Address 0x%x\n", t->da);
> - seq_printf(seq, " Length 0x%x Bytes\n", t->len);
> - seq_printf(seq, " Reserved (should be zero) [%d]\n", t->reserved);
> - seq_printf(seq, " Name %s\n\n", t->name);
> + if (ver == 0) {
Again, this is a switch, here in a switch. Just defining a new
RSC_TRACE64 type would reduce the amount of code here...
> + t1 = rsc;
> + seq_printf(seq, "Entry %d is version %d of type %s\n",
> + i, ver, types[hdr->st.t]);
> + seq_printf(seq, " Device Address 0x%x\n",
> + t1->da);
> + seq_printf(seq, " Length 0x%x Bytes\n",
> + t1->len);
> + seq_printf(seq, " Reserved (should be zero) [%d]\n",
> + t1->reserved);
> + seq_printf(seq, " Name %s\n\n", t1->name);
> + } else if (ver == 1) {
> + t2 = rsc;
> + seq_printf(seq, "Entry %d is version %d of type %s\n",
> + i, ver, types[hdr->st.t]);
> + seq_printf(seq, " Device Address 0x%llx\n",
> + t2->da);
> + seq_printf(seq, " Length 0x%x Bytes\n",
> + t2->len);
> + seq_printf(seq, " Reserved (should be zero) [%d]\n",
> + t2->reserved);
> + seq_printf(seq, " Name %s\n\n", t2->name);
> + } else {
> + seq_printf(seq, "Entry %d is an unsupported version %d of type %s\n",
> + i, ver, types[hdr->st.t]);
> + }
> break;
> case RSC_VDEV:
> v = rsc;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 526d3cb45e37..3b3bea42f8b1 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -243,6 +243,32 @@ struct fw_rsc_trace {
> u8 name[32];
> } __packed;
>
> +/**
> + * struct fw_rsc_trace2 - trace buffer declaration supporting 64-bits
> + * @padding: initial padding after type field for aligned 64-bit access
> + * @da: device address (64-bit)
> + * @len: length (in bytes)
> + * @reserved: reserved (must be zero)
> + * @name: human-readable name of the trace buffer
> + *
> + * This resource entry is an enhanced version of the fw_rsc_trace resourec entry
> + * and the provides equivalent functionality but designed for 64-bit remote
> + * processors.
> + *
> + * @da specifies the device address of the buffer, @len specifies
> + * its size, and @name may contain a human readable name of the trace buffer.
> + *
> + * After booting the remote processor, the trace buffers are exposed to the
> + * user via debugfs entries (called trace0, trace1, etc..).
> + */
> +struct fw_rsc_trace2 {
Sounds more like fw_rsc_trace64 to me - in particular since the version
of trace2 is 1...
> + u32 padding;
> + u64 da;
> + u32 len;
> + u32 reserved;
What's the purpose of this reserved field?
Regards,
Bjorn
> + u8 name[32];
> +} __packed;
> +
> /**
> * struct fw_rsc_vdev_vring - vring descriptor entry
> * @da: device address
> --
> 2.23.0
>
next prev parent reply other threads:[~2020-05-21 18:05 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-25 20:46 [PATCH 0/4] Update K3 DSP remoteproc driver for C71x DSPs Suman Anna
2020-03-25 20:46 ` [PATCH 1/4] dt-bindings: remoteproc: k3-dsp: Update bindings " Suman Anna
2020-03-31 21:56 ` Rob Herring
2020-03-25 20:46 ` [PATCH 2/4] remoteproc: introduce version element into resource type field Suman Anna
2020-05-21 17:54 ` Bjorn Andersson
2020-05-21 19:06 ` Suman Anna
2020-05-21 19:21 ` Bjorn Andersson
2020-05-21 19:29 ` Suman Anna
2020-05-21 19:41 ` Bjorn Andersson
2020-05-21 19:52 ` Suman Anna
2020-03-25 20:47 ` [PATCH 3/4] remoteproc: add support for a new 64-bit trace version Suman Anna
2020-05-21 18:04 ` Bjorn Andersson [this message]
2020-05-21 19:42 ` Suman Anna
2020-05-22 16:54 ` Suman Anna
2020-05-22 17:33 ` Bjorn Andersson
2020-05-22 18:03 ` Clément Leger
2020-05-22 18:10 ` Clément Leger
2020-05-22 18:59 ` Suman Anna
2020-05-22 19:28 ` Clément Leger
2020-03-25 20:47 ` [PATCH 4/4] remoteproc/k3-dsp: Add support for C71x DSPs Suman Anna
2020-04-27 19:54 ` Suman Anna
2020-05-21 15:57 ` [PATCH 0/4] Update K3 DSP remoteproc driver " Suman Anna
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=20200521180417.GJ408178@builder.lan \
--to=bjorn.andersson@linaro.org \
--cc=arnaud.pouliquen@st.com \
--cc=cleger@kalray.eu \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=loic.pallardy@st.com \
--cc=lokeshvutla@ti.com \
--cc=mathieu.poirier@linaro.org \
--cc=robh+dt@kernel.org \
--cc=s-anna@ti.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).