Date: Wed, 19 Jun 2024 14:05:33 +0800
From: JieGan <quic_jiegan@...cinc.com>
To: Suzuki K Poulose <suzuki.poulose@....com>
CC: Mathieu Poirier <mathieu.poirier@...aro.org>,
        Alexander Shishkin
	<alexander.shishkin@...ux.intel.com>,
        Konrad Dybcio <konradybcio@...il.com>,
        Mike Leach <mike.leach@...aro.org>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jinlong Mao
	<quic_jinlmao@...cinc.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <coresight@...ts.linaro.org>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        Tingwei Zhang
	<quic_tingweiz@...cinc.com>,
        Yuanfang Zhang <quic_yuanfang@...cinc.com>,
        "Tao
 Zhang" <quic_taozha@...cinc.com>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        "Song
 Chai" <quic_songchai@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <andersson@...nel.org>,
        <quic_yijiyang@...cinc.com>, <quic_yuanjiey@...cinc.com>,
        <quic_liuxin@...cinc.com>, <quic_yanzl@...cinc.com>,
        <quic_xinlon@...cinc.com>, <quic_xueqnie@...cinc.com>,
        <quic_sijiwu@...cinc.com>
Subject: Re: [PATCH v1 0/3] Add coresight slave register driver to support
 data filter function

On Tue, Jun 18, 2024 at 10:47:31AM +0100, Suzuki K Poulose wrote:
> On 18/06/2024 08:27, Jie Gan wrote:
> > The Coresight Slave Register(CSR) device hosts miscellaneous configuration
> > registers to control various features related to TMC ETR device.
> > 

I would like to provide a full picture about the CSR device(to be renamed)
before get through below comments.

CSR includes some control and status registers. These registers control
several hardware block related to coresight. One example is byte counter 
function which is discussed in below mail thread. 
https://lore.kernel.org/lkml/CAJ9a7Vjj_pcr4bZsrdqTR1=u1RHDZ_t0wgtbdn62a5j64hYrdA@mail.gmail.com/
That hardware can generate interrupt once ETR receive enough data 
so OS has chance to save it in storage. CSR is introduced here as a helper
device here since it controls the trace ID filter in ETR. 

The use case is we have two ETRs in system. We can configure one ETR as sink
for high throughput trace data  like ETM and another ETR for low throughput
trace data like STM. In this case, STM data won’t be flushed out by ETM data quickly.


> > The CSR device works as a helper device physically connected to the TMC ETR device.
> > ---------------------------------------------------------
> >               |ETR0|             |ETR1|
> >                . \                 / .
> >                .  \               /  .
> >                .   \             /   .
> >                .    \           /    .
> > ---------------------------------------------------
> > ETR0ATID0-ETR0ATID3     CSR     ETR1ATID0-ETR1ATID3
> > ---------------------------------------------------
> > Each ETR has four ATID registers with 128 bits long in total.
> > e.g. ETR0ATID0-ETR0ATID3 registers are used by ETR0 device.
> 
> What is the maximum number of connections possible for CSR ? 2 ETRs ?
The maximum number of connections for existing device is 2. But it should support
more than 2 connections and it depends on the hardware design. One more ETR device
added, four related ATID registers need add to CSR device as well.

> 
> > 
> > Based on the trace id which is programed in CSR ATID register of
> > specific ETR, trace data with that trace id can get into ETR's buffer
> 
> How do you handle cases where there are multiple TraceIDs in a the stream ?
> e.g., perf tracing a multi-threaded app ? Each ETM will have
> a distinct traceid. Is there way to disable filtering by CSR ?
Here is an example when enable multiple traceIDs in a stream.
Suppose we will enable three source devices with trace id 1, 2, 3 and these source devices
will enable one by one. First, we enable the source device with trace id 1. It sets bit 1
in ATID register. Then, enable the source device with trace id 2, it sets bit 2 in ATID register.
Finally, the source device with trace id 3 is enabled with set bit 3 in ATID register concurrently.
After the enablement, we have a bitmap for ATID register below:
31          0  
-------------
0.......01110

There is no way to disable filtering by CSR in this version of patch.
I will add a way to disable filtering by simply enable all bits of the register.

> 
> Side note, with James's trace id allocation per sink series makes this
> easier for the ETR to know the trace ids allocated for the current
> session. Works only for perf though.
I just checked James's patch. I will try to re-design the function refer to James's solution.

> 
> 
> > while other trace data gets ignored. CSR may contain several ATID registers.
> > Each ATID register is associated with an ETR device.
> > 
> > To achieve this function, the trace id is obtained and stored in the related
> > ETR device's driver data just before enabling the CSR. Then, the CSR
> > device can easily obtain the trace ID from the ETR's driver data because the
> > ETR's driver data is passed to the CSR's enable/disable functions.
> > 
> > Ensure that every source device has already allocated a trace ID in its probe
> > session because the sink device should always be the first device to
> 
> How is that possible ? We are going backwards in the trace id allocation
> with your proposal. What is the purpose of this hardware when you could use
> a replicator with trace filtering based on masks ?
How to obtain the trace id of the source device and pass it to CSR device is a big issue
that we must deal with. As we know the source device always be the last device to enable.
If the allocation is happened when enabling the source device, we may need a callback function
that is inserted into the enable function of the source device to help active bit in CSR device.

All source devices except for etm are allocated its trace id in probe. So, I just add function
to ensure the ETM device allocates its trace id in probe. The purpose of the modification is
to decrease the impact for current framework.

I will reconsider the way to obtain trace ID. May add a callback function is a better solution or
refer to James's solution.

I don’t think our hardware have replicator trace filtering function.
Is there a new function supported by replicator?

> 
> > enable when operating coresight_enable_path function. As a helper device of the
> > ETR, the CSR device will program the ATID register of a specific ETR according to
> > the trace id to enable data filter function at a very early stage. Without the
> > correct trace ID, the enablement session will not work.
> > 
> > Each CSR's enable session will set one bit in the ATID register.
> 
> So is this a bitmap of "enable/disable" ATID ? I really don't see the
> usecase of the CSR "device" yet. Please could you share "usecase" ?
Please refer to previous explanation.

> 
> Suzuki
>
Thanks,
Jie
 
> 
> > Every CSR's disbale seesion will reset all bits of the ATID register.
> > 
> > This patch only supports sysfs mode. I will send the perf mode part patch
> > once it is ready.
> > 
> > Looking forward to receiving comments as this is a new driver.
> > 
> > Thanks!
> > 
> > Jie Gan (3):
> >    dt-bindings: arm: Add binding document for Coresight Slave Register
> >      device.
> >    coresight: Add coresight slave register driver to support data filter
> >      function in sysfs mode
> >    arm64: dts: qcom: Add CSR and ETR nodes for SA8775p
> > 
> >   .../bindings/arm/arm,coresight-tmc.yaml       |   8 +
> >   .../bindings/arm/qcom,coresight-csr.yaml      |  49 +++
> >   arch/arm64/boot/dts/qcom/sa8775p.dtsi         | 167 ++++++++++
> >   drivers/hwtracing/coresight/Kconfig           |   6 +
> >   drivers/hwtracing/coresight/Makefile          |   1 +
> >   drivers/hwtracing/coresight/coresight-core.c  |   6 +-
> >   drivers/hwtracing/coresight/coresight-csr.c   | 315 ++++++++++++++++++
> >   drivers/hwtracing/coresight/coresight-csr.h   |  24 ++
> >   .../coresight/coresight-etm4x-core.c          |   1 +
> >   drivers/hwtracing/coresight/coresight-stm.c   |  50 ---
> >   drivers/hwtracing/coresight/coresight-sysfs.c |  45 ++-
> >   .../hwtracing/coresight/coresight-tmc-core.c  |   1 +
> >   drivers/hwtracing/coresight/coresight-tmc.h   |   2 +
> >   include/linux/coresight-stm.h                 |  44 +++
> >   14 files changed, 665 insertions(+), 54 deletions(-)
> >   create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-csr.yaml
> >   create mode 100644 drivers/hwtracing/coresight/coresight-csr.c
> >   create mode 100644 drivers/hwtracing/coresight/coresight-csr.h
> > 
> 

Powered by blists - more mailing lists