Message-ID: <3af020da-f61b-4a32-8209-227b8fe58a78@gmail.com>
Date: Fri, 10 Oct 2025 17:41:58 -0700
From: Bo Gan <ganboing@...il.com>
To: Mayuresh Chitale <mchitale@...tanamicro.com>, Bo Gan <ganboing@...il.com>
Cc: Anup Patel <apatel@...tanamicro.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Greg KH <gregkh@...uxfoundation.org>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Ian Rogers <irogers@...gle.com>, Mark Rutland <mark.rutland@....com>,
 devicetree@...r.kernel.org, Alexandre Ghiti <alex@...ti.fr>,
 Atish Patra <atish.patra@...ux.dev>, Peter Zijlstra <peterz@...radead.org>,
 Anup Patel <anup@...infault.org>, Adrian Hunter <adrian.hunter@...el.com>,
 linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
 Jiri Olsa <jolsa@...nel.org>, Mayuresh Chitale <mchitale@...il.com>,
 Namhyung Kim <namhyung@...nel.org>, linux-riscv@...ts.infradead.org,
 Andrew Jones <ajones@...tanamicro.com>, Liang Kan <kan.liang@...ux.intel.com>
Subject: Re: [PATCH 07/11] rvtrace: Add trace ramsink driver

On 10/8/25 03:35, Mayuresh Chitale wrote:
> 
> 
> On Tue, Oct 7, 2025 at 1:21 PM Bo Gan <ganboing@...il.com <mailto:ganboing@...il.com>> wrote:
>  >
>  > On 10/1/25 23:07, Anup Patel wrote:
>  > > From: Mayuresh Chitale <mchitale@...tanamicro.com <mailto:mchitale@...tanamicro.com>>
>  > >
>  > > Add initial implementation of RISC-V trace ramsink driver. The ramsink
>  > > is defined in the RISC-V Trace Control Interface specification.
>  > >
>  > > Co-developed-by: Anup Patel <apatel@...tanamicro.com <mailto:apatel@...tanamicro.com>>
>  > > Signed-off-by: Anup Patel <apatel@...tanamicro.com <mailto:apatel@...tanamicro.com>>
>  > > Signed-off-by: Mayuresh Chitale <mchitale@...tanamicro.com <mailto:mchitale@...tanamicro.com>>
>  > > ---
>  > >   drivers/hwtracing/rvtrace/Kconfig           |   8 +
>  > >   drivers/hwtracing/rvtrace/Makefile          |   1 +
>  > >   drivers/hwtracing/rvtrace/rvtrace-ramsink.c | 198 ++++++++++++++++++++
>  > >   3 files changed, 207 insertions(+)
>  > >   create mode 100644 drivers/hwtracing/rvtrace/rvtrace-ramsink.c
>  > >
>  > > diff --git a/drivers/hwtracing/rvtrace/Kconfig b/drivers/hwtracing/rvtrace/Kconfig
>  > > index ba35c05f3f54..aef7e9989165 100644
>  > > --- a/drivers/hwtracing/rvtrace/Kconfig
>  > > +++ b/drivers/hwtracing/rvtrace/Kconfig
>  > > @@ -21,3 +21,11 @@ config RVTRACE_ENCODER
>  > >       default y
>  > >       help
>  > >         This driver provides support for RISC-V Trace Encoder component.
>  > > +
>  > > +config RVTRACE_RAMSINK
>  > > +     tristate "RISC-V Trace Ramsink driver"
>  > > +     depends on RVTRACE
>  > > +     default y
>  > > +     help
>  > > +       This driver provides support for Risc-V E-Trace Ramsink
>  > > +       component.
>  > > diff --git a/drivers/hwtracing/rvtrace/Makefile b/drivers/hwtracing/rvtrace/Makefile
>  > > index f320693a1fc5..122e575da9fb 100644
>  > > --- a/drivers/hwtracing/rvtrace/Makefile
>  > > +++ b/drivers/hwtracing/rvtrace/Makefile
>  > > @@ -3,3 +3,4 @@
>  > >   obj-$(CONFIG_RVTRACE) += rvtrace.o
>  > >   rvtrace-y := rvtrace-core.o rvtrace-platform.o
>  > >   obj-$(CONFIG_RVTRACE_ENCODER) += rvtrace-encoder.o
>  > > +obj-$(CONFIG_RVTRACE_RAMSINK) += rvtrace-ramsink.o
>  > > diff --git a/drivers/hwtracing/rvtrace/rvtrace-ramsink.c b/drivers/hwtracing/rvtrace/rvtrace-ramsink.c
>  > > new file mode 100644
>  > > index 000000000000..7bd0cf1e4dfd
>  > > --- /dev/null
>  > > +++ b/drivers/hwtracing/rvtrace/rvtrace-ramsink.c
>  > > @@ -0,0 +1,198 @@
>  > > +// SPDX-License-Identifier: GPL-2.0
>  > > +/*
>  > > + * Copyright (c) 2025 Ventana Micro Systems Inc.
>  > > + */
>  > > +
>  > > +#include <linux/device.h>
>  > > +#include <linux/io.h>
>  > > +#include <linux/of.h>
>  > > +#include <linux/of_graph.h>
>  > > +#include <linux/platform_device.h>
>  > > +#include <linux/property.h>
>  > > +#include <linux/dma-mapping.h>
>  > > +#include <linux/rvtrace.h>
>  > > +#include <linux/types.h>
>  > > +#include <linux/sizes.h>
>  > > +
>  > > +#define RVTRACE_RAMSINK_STARTLOW_OFF         0x010
>  > > +#define RVTRACE_RAMSINK_STARTHIGH_OFF                0x014
>  > > +#define RVTRACE_RAMSINK_LIMITLOW_OFF         0x018
>  > > +#define RVTRACE_RAMSINK_LIMITHIGH_OFF                0x01c
>  > > +#define RVTRACE_RAMSINK_WPLOW_OFF            0x020
>  > > +#define RVTRACE_RAMSINK_WPHIGH_OFF           0x024
>  > > +#define RVTRACE_RAMSINK_RPLOW_OFF            0x028
>  > > +#define RVTRACE_RAMSINK_RPHIGH_OFF           0x02c
>  > > +
>  > > +struct rvtrace_ramsink_priv {
>  > > +     size_t size;
>  > > +     void *va;
>  > > +     dma_addr_t start;
>  > > +     dma_addr_t end;
>  > > +     /* WP from prev iteration */
>  > > +     dma_addr_t prev_head;
>  > > +};
>  > > +
>  > > +struct trace_buf {
>  > > +     void *base;
>  > > +     size_t size;
>  > > +     int cur, len;
>  > > +};
>  > > +
>  > > +static void tbuf_to_pbuf_copy(struct trace_buf *src, struct trace_buf *dst)
>  > > +{
>  > > +     int bytes_dst, bytes_src, bytes;
>  > > +     void *dst_addr, *src_addr;
>  > > +
>  > > +     while (src->size) {
>  > > +             src_addr = src->base + src->cur;
>  > > +             dst_addr = dst->base + dst->cur;
>  > > +
>  > > +             if (dst->len - dst->cur < src->size)
>  > > +                     bytes_dst = dst->len - dst->cur;
>  > > +             else
>  > > +                     bytes_dst = src->size;
>  > > +             if (src->len - src->cur < src->size)
>  > > +                     bytes_src = src->len - src->cur;
>  > > +             else
>  > > +                     bytes_src = src->size;
>  > > +             bytes = bytes_dst < bytes_src ? bytes_dst : bytes_src;
>  > > +             memcpy(dst_addr, src_addr, bytes);
>  > > +             dst->cur = (dst->cur + bytes) % dst->len;
>  > > +             src->cur = (src->cur + bytes) % src->len;
>  > > +             src->size -= bytes;
>  > > +     }
>  > > +}
>  > > +
>  > > +static size_t rvtrace_ramsink_copyto_auxbuf(struct rvtrace_component *comp,
>  > > +                                         struct rvtrace_perf_auxbuf *buf)
>  > > +{
>  > > +     struct rvtrace_ramsink_priv *priv = dev_get_drvdata(&comp->dev);
>  > > +     struct trace_buf src, dst;
>  > > +     u32 wp_low, wp_high;
>  > > +     u64 buf_cur_head;
>  > > +     size_t size;
>  > > +
>  > > +     wp_low = rvtrace_read32(comp->pdata, RVTRACE_RAMSINK_WPLOW_OFF);
>  > > +     wp_high = rvtrace_read32(comp->pdata, RVTRACE_RAMSINK_WPHIGH_OFF);
>  > > +     buf_cur_head = (u64)(wp_high) << 32 | wp_low;
>  > > +
>  > > +     if (buf_cur_head == priv->prev_head)
>  > > +             return 0;
>  > > +
>  > > +     dst.base = buf->base;
>  > > +     dst.len = buf->length;
>  > > +     dst.cur = buf->pos;
>  > > +     dst.size = 0;
>  > > +
>  > > +     src.base = priv->va;
>  > > +     src.len = priv->end - priv->start;
>  > > +     if (buf_cur_head > priv->prev_head) {
>  > > +             src.size = buf_cur_head - priv->prev_head;
>  > > +     } else {
>  > > +             src.size = priv->end - priv->prev_head;
>  > > +             src.size += buf_cur_head - priv->start;
>  > > +     }
>  > > +
>  > > +     src.cur = buf_cur_head - priv->start;
>  > > +     size = src.size;
>  > > +     tbuf_to_pbuf_copy(&src, &dst);
>  > > +     buf->pos = dst.cur;
>  > > +     priv->prev_head = buf_cur_head;
>  > > +
>  > > +     return size;
>  > > +}
>  > > +
>  > > +static int rvtrace_ramsink_setup(struct rvtrace_component *comp)
>  > > +{
>  > > +     struct rvtrace_ramsink_priv *priv;
>  > > +
>  > > +     priv = devm_kzalloc(&comp->dev, sizeof(*priv), GFP_KERNEL);
>  > > +     if (!priv)
>  > > +             return -ENOMEM;
>  > > +     dev_set_drvdata(&comp->dev, priv);
>  > > +
>  > > +     priv->size = SZ_4M;
>  > Can we make this size dynamically determined? 4M seems inadequate. This is
>  > exceedingly so if the RAM sink is linked to a funnel, where you can have
>  > many harts dumping traces into this sink.
>  >
>  > > +     priv->va = dma_alloc_coherent(&comp->dev, priv->size, &priv->start, GFP_KERNEL);
>  > > +     if (!priv->va)
>  > > +             return -ENOMEM;
>  > > +     priv->end = priv->start + priv->size;
>  > > +     priv->prev_head = priv->start;
>  > > +
>  > > +     /* Setup ram sink addresses */
>  > > +     rvtrace_write32(comp->pdata, lower_32_bits(priv->start), RVTRACE_RAMSINK_STARTLOW_OFF);
>  > > +     rvtrace_write32(comp->pdata, upper_32_bits(priv->start), RVTRACE_RAMSINK_STARTHIGH_OFF);
>  > > +     rvtrace_write32(comp->pdata, lower_32_bits(priv->start), RVTRACE_RAMSINK_WPLOW_OFF);
>  > > +     rvtrace_write32(comp->pdata, upper_32_bits(priv->start), RVTRACE_RAMSINK_WPHIGH_OFF);
>  > > +     /* Limit address needs to be set to end - 4 so that HW doesn't cause an overflow. */> + rvtrace_write32(comp->pdata, lower_32_bits(priv->end - 0x4), RVTRACE_RAMSINK_LIMITLOW_OFF);
>  > Should not hardcode 4 as the trace write width. Control Interface Spec
>  > chapter 7.1 has the proper instruction on how to probe for this width:
>  >
>  > "Not every value may be settable in trRamStart/Limit registers. Value
>  >   written may be trimmed (for example aligned on a particular 2^N boundary)
>  >   and a trace tool should verify values being written"
>  >
> Thanks for the comments. I will incorporate those in v2.

@Mayuresh, I went over the spec again. I don't see the handling off wrap.
You need to detect if the buffer has wrapped around by testing bit 0 of
trRamWPLow. So:

  a. trRamWPLow needs masking bit 0 before being used as address to read
     trace data.
  b. If wrapped, then the trace buffer is actually scattered in two parts:
     [WP, Limit + width) and [Start, WP). During copying, you need to use
     the right portion of the trace buffer.

ref: https://github.com/ganboing/riscv-trace-umd/blob/master/rvtrace/funnel.py#L260

BTW, again your mail wasn't delivered to the mailing-list for some reason
Please check: I couldn't find your name on

https://lists.infradead.org/pipermail/linux-riscv/2025-October/thread.html
https://lore.kernel.org/all/20251002060732.100213-1-apatel@ventanamicro.com/

> 
>  > > +     rvtrace_write32(comp->pdata, upper_32_bits(priv->end), RVTRACE_RAMSINK_LIMITHIGH_OFF);
>  > > +
>  > > +     return 0;
>  > > +}
>  > > +
>  > > +static void rvtrace_ramsink_cleanup(struct rvtrace_component *comp)
>  > > +{
>  > > +     struct rvtrace_ramsink_priv *priv = dev_get_drvdata(&comp->dev);
>  > > +
>  > > +     dma_free_coherent(&comp->dev, priv->size, priv->va, priv->start);
>  > > +}
>  > > +
>  > > +static int rvtrace_ramsink_probe(struct rvtrace_component *comp)
>  > > +{
>  > > +     int ret;
>  > > +
>  > > +     ret = rvtrace_ramsink_setup(comp);
>  > > +     if (ret)
>  > > +             return dev_err_probe(&comp->dev, ret, "failed to setup ramsink.\n");
>  > > +
>  > > +     ret = rvtrace_enable_component(comp);
>  > > +     if (ret)
>  > > +             return dev_err_probe(&comp->dev, ret, "failed to enable ramsink.\n");
>  > > +
>  > > +     return ret;
>  > > +}
>  > > +
>  > > +static void rvtrace_ramsink_remove(struct rvtrace_component *comp)
>  > > +{
>  > > +     int ret;
>  > > +
>  > > +     ret = rvtrace_disable_component(comp);
>  > > +     if (ret)
>  > > +             dev_err(&comp->dev, "failed to disable ramsink.\n");
>  > > +
>  > > +     rvtrace_ramsink_cleanup(comp);
>  > > +}
>  > > +
>  > > +static struct rvtrace_component_id rvtrace_ramsink_ids[] = {
>  > > +     { .type = RVTRACE_COMPONENT_TYPE_RAMSINK,
>  > > +       .version = rvtrace_component_mkversion(1, 0), },
>  > > +     {},
>  > > +};
>  > > +
>  > > +static struct rvtrace_driver rvtrace_ramsink_driver = {
>  > > +     .id_table = rvtrace_ramsink_ids,
>  > > +     .copyto_auxbuf = rvtrace_ramsink_copyto_auxbuf,
>  > > +     .probe = rvtrace_ramsink_probe,
>  > > +     .remove = rvtrace_ramsink_remove,
>  > > +     .driver = {
>  > > +             .name = "rvtrace-ramsink",
>  > > +     },
>  > > +};
>  > > +
>  > > +static int __init rvtrace_ramsink_init(void)
>  > > +{
>  > > +     return rvtrace_register_driver(&rvtrace_ramsink_driver);
>  > > +}
>  > > +
>  > > +static void __exit rvtrace_ramsink_exit(void)
>  > > +{
>  > > +     rvtrace_unregister_driver(&rvtrace_ramsink_driver);
>  > > +}
>  > > +
>  > > +module_init(rvtrace_ramsink_init);
>  > > +module_exit(rvtrace_ramsink_exit);
>  > > +
>  > > +/* Module information */
>  > > +MODULE_AUTHOR("Mayuresh Chitale <mchitale@...tanamicro.com <mailto:mchitale@...tanamicro.com>>");
>  > > +MODULE_DESCRIPTION("RISC-V Trace Ramsink Driver");
>  > > +MODULE_LICENSE("GPL");
>  >
>  > Bo

Bo

Powered by blists - more mailing lists