| Message ID | 20200325204701.16862-4-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update K3 DSP remoteproc driver for C71x DSPs | expand |
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 >
Hi Bjorn, On 5/21/20 1:04 PM, Bjorn Andersson wrote: > 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. Yeah, ok. Will refactor for this in v2. > >> + 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... OK. > >> + 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... Yeah, will rename this. > >> + u32 padding; >> + u64 da; >> + u32 len; >> + u32 reserved; > > What's the purpose of this reserved field? Partly to make sure the entire resource is aligned on an 8-byte, and partly copied over from fw_rsc_trace entry. I guess 32-bits is already large enough of a size for trace entries irrespective of 32-bit or 64-bit traces, so I doubt if we want to make the len field also a u64. regards Suman > > Regards, > Bjorn > >> + u8 name[32]; >> +} __packed; >> + >> /** >> * struct fw_rsc_vdev_vring - vring descriptor entry >> * @da: device address >> -- >> 2.23.0 >>
On 5/21/20 2:42 PM, Suman Anna wrote: > Hi Bjorn, > > On 5/21/20 1:04 PM, Bjorn Andersson wrote: >> 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. > > Yeah, ok. Will refactor for this in v2. > >> >>> + 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... > > OK. > >> >>> + 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... > > Yeah, will rename this. > >> >>> + u32 padding; >>> + u64 da; >>> + u32 len; >>> + u32 reserved; >> >> What's the purpose of this reserved field? > > Partly to make sure the entire resource is aligned on an 8-byte, and > partly copied over from fw_rsc_trace entry. I guess 32-bits is already > large enough of a size for trace entries irrespective of 32-bit or > 64-bit traces, so I doubt if we want to make the len field also a u64. Looking at this again, I can drop both padding and reserved fields, if I move the len field before da. Any preferences/comments? regards Suman > > regards > Suman > >> >> Regards, >> Bjorn >> >>> + u8 name[32]; >>> +} __packed; >>> + >>> /** >>> * struct fw_rsc_vdev_vring - vring descriptor entry >>> * @da: device address >>> -- >>> 2.23.0 >>> >
On Fri 22 May 09:54 PDT 2020, Suman Anna wrote: > On 5/21/20 2:42 PM, Suman Anna wrote: > > Hi Bjorn, > > > > On 5/21/20 1:04 PM, Bjorn Andersson wrote: > > > On Wed 25 Mar 13:47 PDT 2020, Suman Anna wrote: [..] > > > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h [..] > > > > +struct fw_rsc_trace2 { > > > > > > Sounds more like fw_rsc_trace64 to me - in particular since the version > > > of trace2 is 1... > > > > Yeah, will rename this. > > > > > > > > > + u32 padding; > > > > + u64 da; > > > > + u32 len; > > > > + u32 reserved; > > > > > > What's the purpose of this reserved field? > > > > Partly to make sure the entire resource is aligned on an 8-byte, and > > partly copied over from fw_rsc_trace entry. I guess 32-bits is already > > large enough of a size for trace entries irrespective of 32-bit or > > 64-bit traces, so I doubt if we want to make the len field also a u64. > > Looking at this again, I can drop both padding and reserved fields, if I > move the len field before da. Any preferences/comments? > Sounds good to me. Thanks, Bjorn
Hi Suman, ----- On 22 May, 2020, at 19:33, Bjorn Andersson bjorn.andersson@linaro.org wrote: > On Fri 22 May 09:54 PDT 2020, Suman Anna wrote: > >> On 5/21/20 2:42 PM, Suman Anna wrote: >> > Hi Bjorn, >> > >> > On 5/21/20 1:04 PM, Bjorn Andersson wrote: >> > > On Wed 25 Mar 13:47 PDT 2020, Suman Anna wrote: > [..] >> > > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > [..] >> > > > +struct fw_rsc_trace2 { >> > > >> > > Sounds more like fw_rsc_trace64 to me - in particular since the version >> > > of trace2 is 1... >> > >> > Yeah, will rename this. >> > >> > > >> > > > + u32 padding; >> > > > + u64 da; >> > > > + u32 len; >> > > > + u32 reserved; >> > > >> > > What's the purpose of this reserved field? >> > >> > Partly to make sure the entire resource is aligned on an 8-byte, and >> > partly copied over from fw_rsc_trace entry. I guess 32-bits is already >> > large enough of a size for trace entries irrespective of 32-bit or >> > 64-bit traces, so I doubt if we want to make the len field also a u64. >> >> Looking at this again, I can drop both padding and reserved fields, if I >> move the len field before da. Any preferences/comments? Not only the in structure alignment matters but also in the resource table. Since the resource table is often packed (see [1] for instance), if a [1] https://github.com/OpenAMP/open-amp/blob/master/apps/machine/zynqmp_r5/rsc_table.h >> > > Sounds good to me. > > Thanks, > Bjorn
----- On 22 May, 2020, at 20:03, Clément Leger cleger@kalray.eu wrote: > Hi Suman, > > ----- On 22 May, 2020, at 19:33, Bjorn Andersson bjorn.andersson@linaro.org > wrote: > >> On Fri 22 May 09:54 PDT 2020, Suman Anna wrote: >> >>> On 5/21/20 2:42 PM, Suman Anna wrote: >>> > Hi Bjorn, >>> > >>> > On 5/21/20 1:04 PM, Bjorn Andersson wrote: >>> > > On Wed 25 Mar 13:47 PDT 2020, Suman Anna wrote: >> [..] >>> > > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> [..] >>> > > > +struct fw_rsc_trace2 { >>> > > >>> > > Sounds more like fw_rsc_trace64 to me - in particular since the version >>> > > of trace2 is 1... >>> > >>> > Yeah, will rename this. >>> > >>> > > >>> > > > + u32 padding; >>> > > > + u64 da; >>> > > > + u32 len; >>> > > > + u32 reserved; >>> > > >>> > > What's the purpose of this reserved field? >>> > >>> > Partly to make sure the entire resource is aligned on an 8-byte, and >>> > partly copied over from fw_rsc_trace entry. I guess 32-bits is already >>> > large enough of a size for trace entries irrespective of 32-bit or >>> > 64-bit traces, so I doubt if we want to make the len field also a u64. >>> >>> Looking at this again, I can drop both padding and reserved fields, if I >>> move the len field before da. Any preferences/comments? > Sorry, my message went a bit too fast... So as I was saying: Not only the in-structure alignment matters but also in the resource table. Since the resource table is often packed (see [1] for instance), if a trace resource is embedded in the resource table after another resource aligned on 32 bits only, your 64 bits trace field will potentially end up misaligned. To overcome this, there is multiple solutions: - Split the 64 bits fields into 32bits low and high parts: Since all resources are aligned on 32bits, it will be ok - Use memcpy_from/to_io when reading/writing such fields As I said in a previous message this should probably be used since the memories that are accessed by rproc are io mem (ioremap in almost all drivers). Regards, Clément [1] https://github.com/OpenAMP/open-amp/blob/master/apps/machine/zynqmp_r5/rsc_table.h > > > > >>> >> >> Sounds good to me. >> >> Thanks, > > Bjorn
Hi Clement, > > ----- On 22 May, 2020, at 20:03, Clément Leger cleger@kalray.eu wrote:> >> Hi Suman, >> >> ----- On 22 May, 2020, at 19:33, Bjorn Andersson bjorn.andersson@linaro.org >> wrote: >> >>> On Fri 22 May 09:54 PDT 2020, Suman Anna wrote: >>> >>>> On 5/21/20 2:42 PM, Suman Anna wrote: >>>>> Hi Bjorn, >>>>> >>>>> On 5/21/20 1:04 PM, Bjorn Andersson wrote: >>>>>> On Wed 25 Mar 13:47 PDT 2020, Suman Anna wrote: >>> [..] >>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >>> [..] >>>>>>> +struct fw_rsc_trace2 { >>>>>> >>>>>> Sounds more like fw_rsc_trace64 to me - in particular since the version >>>>>> of trace2 is 1... >>>>> >>>>> Yeah, will rename this. >>>>> >>>>>> >>>>>>> + u32 padding; >>>>>>> + u64 da; >>>>>>> + u32 len; >>>>>>> + u32 reserved; >>>>>> >>>>>> What's the purpose of this reserved field? >>>>> >>>>> Partly to make sure the entire resource is aligned on an 8-byte, and >>>>> partly copied over from fw_rsc_trace entry. I guess 32-bits is already >>>>> large enough of a size for trace entries irrespective of 32-bit or >>>>> 64-bit traces, so I doubt if we want to make the len field also a u64. >>>> >>>> Looking at this again, I can drop both padding and reserved fields, if I >>>> move the len field before da. Any preferences/comments? >> > Sorry, my message went a bit too fast... So as I was saying: > > Not only the in-structure alignment matters but also in the resource table. > Since the resource table is often packed (see [1] for instance), if a trace > resource is embedded in the resource table after another resource aligned > on 32 bits only, your 64 bits trace field will potentially end up > misaligned. Right. Since one can mix and match the resources of different sizes and include them in any order, the onus is going to be on the resource table constructors to ensure the inter-resource alignments, if any are required. The resource table format allows you to add padding fields in between if needed, and the remoteproc core relies on the offsets. I can only ensure the alignment within this resource structure with ready-available access and conversion to/from a 64-bit type, as long as the resource is starting on a 64-bit boundary. > > To overcome this, there is multiple solutions: > > - Split the 64 bits fields into 32bits low and high parts: > Since all resources are aligned on 32bits, it will be ok Yes, this is one solution. At the same time, this means you need additional conversion logic for converting to and from 64-bit field. In this particular case, da is the address of the trace buffer pointer on a 64-bit processor, so we can directly use the address of the trace buffer. Guess it is a question of easier translation vs packing the resource table as tight as possible. > > - Use memcpy_from/to_io when reading/writing such fields > As I said in a previous message this should probably be used since > the memories that are accessed by rproc are io mem (ioremap in almost > all drivers). Anything running out of DDR actually doesn't need the io mem semantics, so we actually need to be fixing the drivers. Blindly changing the current memcpy to memcpy_to_io in the core loader is also not right. Any internal memories properties will actually depend on the processor and SoC. Eg: The R5 TCM interfaces in general can be treated as normal memories. regards Suman > > Regards, > > Clément > > [1] https://github.com/OpenAMP/open-amp/blob/master/apps/machine/zynqmp_r5/rsc_table.h >> >> >> >> >>>> >>> >>> Sounds good to me. >>> >>> Thanks, >>> Bjorn
Hi Suman, ----- On 22 May, 2020, at 20:59, s-anna s-anna@ti.com wrote: > Hi Clement, > >> > ----- On 22 May, 2020, at 20:03, Clément Leger cleger@kalray.eu wrote:> >>> Hi Suman, >>> >>> ----- On 22 May, 2020, at 19:33, Bjorn Andersson bjorn.andersson@linaro.org >>> wrote: >>> >>>> On Fri 22 May 09:54 PDT 2020, Suman Anna wrote: >>>> >>>>> On 5/21/20 2:42 PM, Suman Anna wrote: >>>>>> Hi Bjorn, >>>>>> >>>>>> On 5/21/20 1:04 PM, Bjorn Andersson wrote: >>>>>>> On Wed 25 Mar 13:47 PDT 2020, Suman Anna wrote: >>>> [..] >>>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >>>> [..] >>>>>>>> +struct fw_rsc_trace2 { >>>>>>> >>>>>>> Sounds more like fw_rsc_trace64 to me - in particular since the version >>>>>>> of trace2 is 1... >>>>>> >>>>>> Yeah, will rename this. >>>>>> >>>>>>> >>>>>>>> + u32 padding; >>>>>>>> + u64 da; >>>>>>>> + u32 len; >>>>>>>> + u32 reserved; >>>>>>> >>>>>>> What's the purpose of this reserved field? >>>>>> >>>>>> Partly to make sure the entire resource is aligned on an 8-byte, and >>>>>> partly copied over from fw_rsc_trace entry. I guess 32-bits is already >>>>>> large enough of a size for trace entries irrespective of 32-bit or >>>>>> 64-bit traces, so I doubt if we want to make the len field also a u64. >>>>> >>>>> Looking at this again, I can drop both padding and reserved fields, if I >>>>> move the len field before da. Any preferences/comments? >>> >> Sorry, my message went a bit too fast... So as I was saying: >> >> Not only the in-structure alignment matters but also in the resource table. >> Since the resource table is often packed (see [1] for instance), if a trace >> resource is embedded in the resource table after another resource aligned >> on 32 bits only, your 64 bits trace field will potentially end up >> misaligned. > > Right. Since one can mix and match the resources of different sizes and > include them in any order, the onus is going to be on the resource table > constructors to ensure the inter-resource alignments, if any are > required. The resource table format allows you to add padding fields in > between if needed, and the remoteproc core relies on the offsets. Agreed > > I can only ensure the alignment within this resource structure with > ready-available access and conversion to/from a 64-bit type, as long as > the resource is starting on a 64-bit boundary. > >> >> To overcome this, there is multiple solutions: >> >> - Split the 64 bits fields into 32bits low and high parts: >> Since all resources are aligned on 32bits, it will be ok > > Yes, this is one solution. At the same time, this means you need > additional conversion logic for converting to and from 64-bit field. In > this particular case, da is the address of the trace buffer pointer on a > 64-bit processor, so we can directly use the address of the trace > buffer. Guess it is a question of easier translation vs packing the > resource table as tight as possible. You simply have to add two wrapper such as the following: static inline void rproc_rsc_set_addr(u32 *low, u32 *hi, u64 val) { *low = lower_32_bits(val); *hi = upper_32_bits(val); } static inline u64 rproc_rsc_get_addr(u32 low, u32 hi) { return ((u64) hi << 32) | low; } This is not really difficult to use and will ensure your new trace resource can be used easily without breaking 32 bits alignment. Breaking compatibility is an option also and it is probably needed to document it clearly if it is chosen to do so. > >> >> - Use memcpy_from/to_io when reading/writing such fields >> As I said in a previous message this should probably be used since >> the memories that are accessed by rproc are io mem (ioremap in almost >> all drivers). > > Anything running out of DDR actually doesn't need the io mem semantics, > so we actually need to be fixing the drivers. Blindly changing the > current memcpy to memcpy_to_io in the core loader is also not right. Any > internal memories properties will actually depend on the processor and > SoC. Eg: The R5 TCM interfaces in general can be treated as normal memories. Agreed, this is most of the case indeed where the memories are actually accessible directly. But using ioremap potentially creates a mapping that does not support misaligned accesses and so accesses should be always aligned. memcpy_from/to_io ensures that. IMHO, there is probably something to be rework since the drivers are mapping the memories but the core is accessing this memory, knowing nothing about how it was mapped. Regards, Clément > > regards > Suman > >> >> Regards, >> >> Clément >> >> [1] >> https://github.com/OpenAMP/open-amp/blob/master/apps/machine/zynqmp_r5/rsc_table.h >>> >>> >>> >>> >>>>> >>>> >>>> Sounds good to me. >>>> >>>> Thanks, > >>> Bjorn
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) { + 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); + 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) { + 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 { + u32 padding; + u64 da; + u32 len; + u32 reserved; + u8 name[32]; +} __packed; + /** * struct fw_rsc_vdev_vring - vring descriptor entry * @da: device address
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(-)