Message-ID: <6c7dfe6b-8cc0-4cde-945b-c423ef517be8@gmail.com>
Date: Wed, 8 Oct 2025 01:48:20 -0700
From: Bo Gan <ganboing@...il.com>
To: 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>
Cc: 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,
 Mayuresh Chitale <mchitale@...tanamicro.com>, 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 05/11] rvtrace: Add trace encoder driver

On 10/7/25 00:09, Bo Gan wrote:
> On 10/1/25 23:07, Anup Patel wrote:
>> From: Mayuresh Chitale <mchitale@...tanamicro.com>
>>
>> Add initial implementation of RISC-V trace encoder driver. The encoder
>> is defined in the RISC-V Trace Control Interface specification.
>>
>> Co-developed-by: Anup Patel <apatel@...tanamicro.com>
>> Signed-off-by: Anup Patel <apatel@...tanamicro.com>
>> Signed-off-by: Mayuresh Chitale <mchitale@...tanamicro.com>
>> ---
>>   drivers/hwtracing/rvtrace/Kconfig           |   7 ++
>>   drivers/hwtracing/rvtrace/Makefile          |   1 +
>>   drivers/hwtracing/rvtrace/rvtrace-encoder.c | 107 ++++++++++++++++++++
>>   3 files changed, 115 insertions(+)
>>   create mode 100644 drivers/hwtracing/rvtrace/rvtrace-encoder.c
>>
>> diff --git a/drivers/hwtracing/rvtrace/Kconfig b/drivers/hwtracing/rvtrace/Kconfig
>> index f8f6feea1953..ba35c05f3f54 100644
>> --- a/drivers/hwtracing/rvtrace/Kconfig
>> +++ b/drivers/hwtracing/rvtrace/Kconfig
>> @@ -14,3 +14,10 @@ menuconfig RVTRACE
>>         To compile this driver as a module, choose M here: the module
>>         will be called rvtrace.
>> +
>> +config RVTRACE_ENCODER
>> +    tristate "RISC-V Trace Encoder driver"
>> +    depends on RVTRACE
>> +    default y
>> +    help
>> +      This driver provides support for RISC-V Trace Encoder component.
>> diff --git a/drivers/hwtracing/rvtrace/Makefile b/drivers/hwtracing/rvtrace/Makefile
>> index 988525a379cf..f320693a1fc5 100644
>> --- a/drivers/hwtracing/rvtrace/Makefile
>> +++ b/drivers/hwtracing/rvtrace/Makefile
>> @@ -2,3 +2,4 @@
>>   obj-$(CONFIG_RVTRACE) += rvtrace.o
>>   rvtrace-y := rvtrace-core.o rvtrace-platform.o
>> +obj-$(CONFIG_RVTRACE_ENCODER) += rvtrace-encoder.o
>> diff --git a/drivers/hwtracing/rvtrace/rvtrace-encoder.c b/drivers/hwtracing/rvtrace/rvtrace-encoder.c
>> new file mode 100644
>> index 000000000000..45d1c5b12c51
>> --- /dev/null
>> +++ b/drivers/hwtracing/rvtrace/rvtrace-encoder.c
>> @@ -0,0 +1,107 @@
>> +// 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/rvtrace.h>
>> +#include <linux/types.h>
>> +
>> +#define RVTRACE_COMPONENT_CTRL_ITRACE_MASK    0x1
>> +#define RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT    2
>> +#define RVTRACE_COMPONENT_CTRL_INSTMODE_MASK    0x7
>> +#define RVTRACE_COMPONENT_CTRL_INSTMODE_SHIFT    4
>> +
>> +static int rvtrace_encoder_start(struct rvtrace_component *comp)
>> +{
>> +    u32 val;
>> +
>> +    val = rvtrace_read32(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
>> +    val |= BIT(RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT);
>> +    rvtrace_write32(comp->pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
>> +    return rvtrace_poll_bit(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
>> +                RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT, 1,
>> +                comp->pdata->control_poll_timeout_usecs);
>> +}
>> +
>> +static int rvtrace_encoder_stop(struct rvtrace_component *comp)
>> +{
>> +    u32 val;
>> +
>> +    val = rvtrace_read32(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
>> +    val &= ~BIT(RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT);
>> +    rvtrace_write32(comp->pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
>> +    return rvtrace_poll_bit(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
>> +                RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT, 0,
>> +                comp->pdata->control_poll_timeout_usecs);
>> +}
>> +
>> +static void rvtrace_encoder_setmode(struct rvtrace_component *comp, u32 mode)
>> +{
>> +    u32 val;
>> +
>> +    val = rvtrace_read32(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
>> +    val |= (mode << RVTRACE_COMPONENT_CTRL_INSTMODE_SHIFT);
>> +    rvtrace_write32(comp->pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
>> +}
>> +
>> +static int rvtrace_encoder_probe(struct rvtrace_component *comp)
>> +{
>> +    int ret;
>> +
>> +    rvtrace_encoder_setmode(comp, 0x6);
>> +    ret = rvtrace_enable_component(comp);
>> +    if (ret)
>> +        return dev_err_probe(&comp->dev, ret, "failed to enable encoder.\n");
>> +
>> +    return 0;
>> +}
> Trace components needs proper probing before use. Refer to Control Interface
> Spec: https://github.com/riscv-non-isa/tg-nexus-trace/releases/download/1.0_Ratified/RISC-V-Trace-Control-Interface.pdf
> Chapter 5. This versioning applies to all components, e.g., sinks/funnels...
> The driver should check the HW impl version with what it supports, and
> rejects registering the component(s) if not supported. Chapter 5. has more
> details.
> 
Forgot to mention another thing: you also need to follow Control Interface
Spec Chapter 11.2 Reset and Discovery to toggle tr??Active. Do not access
any other registers or set any other bits in tr??Control before properly
enable tr??Active. My userspace driver does the reset as:
https://github.com/ganboing/riscv-trace-umd/blob/master/rvtrace/device.py#L57

Your HW IP might ignore tr??Active bit and the trace components are always
clocked, and that's probably why you don't see any issue without proper
resetting. However, on my p550 (EIC7700), I adopted your code to a v6.6
tree, boot, and load it as module. The core immediately hangs, very likely
due to it tries to set tr??Control bits without proper resetting. You
should expect other IPs that implement clock gating and strictly follow
the Spec. Thanks.

>> +
>> +static void rvtrace_encoder_remove(struct rvtrace_component *comp)
>> +{
>> +    int ret;
>> +
>> +    ret = rvtrace_disable_component(comp);
>> +    if (ret)
>> +        dev_err(&comp->dev, "failed to disable encoder.\n");
>> +}
>> +
>> +static struct rvtrace_component_id rvtrace_encoder_ids[] = {
>> +    { .type = RVTRACE_COMPONENT_TYPE_ENCODER,
>> +      .version = rvtrace_component_mkversion(1, 0), },
>> +    {},
>> +};
>> +
>> +static struct rvtrace_driver rvtrace_encoder_driver = {
>> +    .id_table = rvtrace_encoder_ids,
>> +    .start = rvtrace_encoder_start,
>> +    .stop = rvtrace_encoder_stop,
>> +    .probe = rvtrace_encoder_probe,
>> +    .remove = rvtrace_encoder_remove,
>> +    .driver = {
>> +        .name = "rvtrace-encoder",
>> +    },
>> +};
>> +
>> +static int __init rvtrace_encoder_init(void)
>> +{
>> +    return rvtrace_register_driver(&rvtrace_encoder_driver);
>> +}
>> +
>> +static void __exit rvtrace_encoder_exit(void)
>> +{
>> +    rvtrace_unregister_driver(&rvtrace_encoder_driver);
>> +}
>> +
>> +module_init(rvtrace_encoder_init);
>> +module_exit(rvtrace_encoder_exit);
>> +
>> +/* Module information */
>> +MODULE_AUTHOR("Mayuresh Chitale <mchitale@...tanamicro.com>");
>> +MODULE_DESCRIPTION("RISC-V Trace Encoder Driver");
>> +MODULE_LICENSE("GPL");
> 
> Bo

Bo

Powered by blists - more mailing lists