Message-ID: <1557f208-1fb7-095a-f57c-9509cbc62aaa@quicinc.com>
Date:   Fri, 14 Jul 2023 13:50:23 +0800
From:   Tao Zhang <quic_taozha@...cinc.com>
To:     Suzuki K Poulose <suzuki.poulose@....com>,
        Mike Leach <mike.leach@...aro.org>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Konrad Dybcio <konradybcio@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jinlong Mao <quic_jinlmao@...cinc.com>,
        Leo Yan <leo.yan@...aro.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>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Hao Zhang <quic_hazha@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <andersson@...nel.org>
Subject: Re: [PATCH v6 09/13] Add nodes for dsb edge control


On 7/14/2023 12:37 AM, Suzuki K Poulose wrote:
> On 13/07/2023 17:13, Tao Zhang wrote:
>>
>> On 7/13/2023 5:34 PM, Suzuki K Poulose wrote:
>>> On 13/07/2023 09:54, Mike Leach wrote:
>>>> HI Tao,
>>>>
>>>> On Wed, 12 Jul 2023 at 14:53, Tao Zhang <quic_taozha@...cinc.com> 
>>>> wrote:
>>>>>
>>>>>
>>>>> On 6/20/2023 9:41 PM, Suzuki K Poulose wrote:
>>>>>> On 20/06/2023 09:31, Tao Zhang wrote:
>>>>>>>
>>>>>>> On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
>>>>>>>> On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
>>>>>>>>> Add the nodes to set value for DSB edge control and DSB edge
>>>>>>>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>>>>>>>>> resgisters to configure edge control. DSB edge detection control
>>>>>>>>> 00: Rising edge detection
>>>>>>>>> 01: Falling edge detection
>>>>>>>>> 10: Rising and falling edge detection (toggle detection)
>>>>>>>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR 
>>>>>>>>> registers to
>>>>>>>>> configure mask. Eight 32 bit registers providing DSB interface
>>>>>>>>> edge detection mask control.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@...cinc.com>
>>>>>>>>> ---
>>>>>>>>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm |  32 +++++
>>>>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.c | 143
>>>>>>>>> ++++++++++++++++++++-
>>>>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.h | 22 ++++
>>>>>>>>>    3 files changed, 196 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> index 2a82cd0..34189e4a 100644
>>>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> @@ -60,3 +60,35 @@ Description:
>>>>>>>>>            Bit[3] : Set to 0 for low performance mode.
>>>>>>>>>                     Set to 1 for high performance mode.
>>>>>>>>>            Bit[4:8] : Select byte lane for high performance mode.
>>>>>>>>> +
>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>>>>>>>>> +Date:        March 2023
>>>>>>>>> +KernelVersion    6.5
>>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@...cinc.com>, Tao
>>>>>>>>> Zhang (QUIC) <quic_taozha@...cinc.com>
>>>>>>>>> +Description:
>>>>>>>>> +        Read/Write a set of the edge control registers of the 
>>>>>>>>> DSB
>>>>>>>>> +        in TPDM.
>>>>>>>>> +
>>>>>>>>> +        Expected format is the following:
>>>>>>>>> +        <integer1> <integer2> <integer3>
>>>>>>>> sysfs is "one value", not 3.  Please never have to parse a 
>>>>>>>> sysfs file.
>>>>>>>
>>>>>>> Do you mean sysfs file can only accept "one value"?
>>>>>>>
>>>>>>> I see that more than one value are written to the sysfs file
>>>>>>> "trigout_attach".
>>>>>>>
>>>>>>>>
>>>>>>>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>>>>>>>>> +                       struct device_attribute *attr,
>>>>>>>>> +                       char *buf)
>>>>>>>>> +{
>>>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>>>> +    ssize_t size = 0;
>>>>>>>>> +    unsigned long bytes;
>>>>>>>>> +    int i;
>>>>>>>>> +
>>>>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>>>>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>>>>>>>>> +        bytes = sysfs_emit_at(buf, size,
>>>>>>>>> +                  "Index:0x%x Val:0x%x\n", i,
>>>>>>>> Again, no, one value, no "string" needed to parse anything.
>>>>>>>
>>>>>>> I also see other sysfs files can be read more than one value in 
>>>>>>> other
>>>>>>> drivers.
>>>>>>>
>>>>>>> Is this "one value" limitation the usage rule of Linux sysfs 
>>>>>>> system?
>>>>>>>
>>>>>>> Or am I misunderstanding what you mean?
>>>>>>
>>>>>> Please fix the other sysfs tunables in the following patches.
>>>>>
>>>>> List a new solution for the similar cases below, please see if this
>>>>> design is reasonable?
>>>>>
>>>>> 1. Two SysFS files("dsb_edge_ctrl_idx" and "dsb_edge_ctrl_val") 
>>>>> will be
>>>>> created in this case.
>>>>>
>>>>> 2. First write to the node "dsb_edge_ctrl_idx" to set the index 
>>>>> number
>>>>> of the edge detection.
>>>>>
>>>>> 3. Then write to the node "dsb_edge_ctrl_val" to set the value of the
>>>>> edge detection.
>>>>>
>>>>> For example, if we need need to set "Falling edge detection" to 
>>>>> the edge
>>>>> detection #220-#222, we can issue the following commands.
>>>>>
>>>>> echo 0xdc > tpdm1/dsb_edge_ctrl_idx
>>>>>
>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>
>>>>> echo 0xdd > tpdm1/dsb_edge_ctrl_idx
>>>>>
>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>
>>>>> echo 0xde > tpdm1/dsb_edge_ctrl_idx
>>>>>
>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>
>>>>> If this design is acceptable, we will rewrite other similar nodes 
>>>>> based
>>>>> on this solution.
>>>>>
>>>>
>>>> This index / value model is used in the coresight drivers so should be
>>>> OK - eg etm4 has cntr_idx / cntrldvr / cntr_val / cntr_ctrl, where
>>>> index selects the counter, and the other val registers are applied to
>>>> that counter.
>>>
>>> True. That model is useful when there are variable number of 
>>> "counters".
>>> I guess it doesn't hurt to have a 64bit (or even 32bit) file for each
>>> EDCR.
>>>
>>> e.g, edcr0...edcr15
>>>
>>> Given there are only 16 of them, it is fine to keep a file for each.
>>> This may be grouped under "mgmt" similar to what we have for other
>>> components. That way, it can be easily hidden by checking for the
>>> presence of DSB.
>>
>> The number of EDCR registers is not fixed. The maximum range is [0:15].
>>
>> But the address of the maximum number of the registers will be 
>> reserved first,
>>
>> and can be accessed safely even if the TPDM doesn't have the maximum 
>> number
>>
>> of  EDCR registers.
>>
>> And we are not able to dynamically know the number of EDCR registers 
>> per DSB
>>
>> TPDM.
>>
>> Can we use our proposal in this case?
>
> Please provide a file edcrN for each of the 0 <= N < 16. That way it is
> easier to avoid locking the index. It doesn't matter how many EDCRs are
> supported, there is a maximum limit and it is always guaranteed to be
> write safe, if some are not implemented. Thus it is much easier from a 
> programming perspective too.

Hi Suzuki,


Thanks for the suggestion.

I'd like to further clarify our proposal below in case I didn't express 
it clearly before.

1. In our design, the users don't need to know the mapping between the 
number of the edge detection

and the control bits in EDCRN registers. They only need to focus on the 
edge detection they need, don't

need to care about the design of the HW.

2. For example, if there are two users configure in the same test. One 
needs to configure edge detection #7

as "Falling edge detection". The other one needs to configure edge 
detection #8 as "toggle detection". They will

issue the following commands to implement it.

echo 0x7 > tpdm1/dsb_edge_ctrl_idx

echo 0x1 > tpdm1/dsb_edge_ctrl_val

echo 0x8 > tpdm1/dsb_edge_ctrl_idx

echo 0x2 > tpdm1/dsb_edge_ctrl_val

The value written to edcr0 will be 0x24000 in our proposal.

But in the solution of "tpdm1/dsb_edge_ctrl/edcrN 0 <= N < 16".

One user calculates that he needs to write 0x4000 to edcr0.

echo 0x4000 > tpdm1/dsb_edge_ctrl/edcr0

The other one calculates that he needs to write 0x20000 to edcr0.

echo 0x20000 > tpdm1/dsb_edge_ctrl/edcr0

The last write will overwrite the previous value in this case and 0x20000

will be written to the edcr0 finally.

3. Some DSB TPDMs may not have 16 EDCR registers. For example, TPDM2

may only have 7 EDCR registers. If we still create 16 edcr file at 
tpdm2/dsb_edge_ctrl,

this may confuse users.

Based on the above points, is it possible to re-evaluate our proposal?


Best,

Tao

>
> Suzuki
>
>
>
>>
>>
>> Best,
>>
>> Tao
>>
>>>
>>> Suzuki
>>>
>

Powered by blists - more mailing lists