by Suzuki K Poulose

[permalink] [raw]

Subject: Re: [PATCH 10/11] coresight: sink: Add TRBE driver

On 1/6/21 11:50 AM, Anshuman Khandual wrote:
>
>
> On 1/5/21 5:07 PM, Suzuki K Poulose wrote:
>> On 1/5/21 9:29 AM, Anshuman Khandual wrote:

...

>>>>> +{
>>>>> +    struct trbe_buf *buf = etm_perf_sink_config(handle);
>>>>> +    unsigned long offset;
>>>>> +
>>>>> +    if (buf->snapshot)
>>>>> +        offset = trbe_snapshot_offset(handle);
>>>>> +    else
>>>>> +        offset = trbe_normal_offset(handle);
>>>>> +    return buf->trbe_base + offset;
>>>>> +}
>>>>> +
>>>>> +static void clear_trbe_state(void)
>>>>
>>>> nit: The name doesn't give much clue about what it is doing, especially, given
>>>> the following "set_trbe_state()" which does completely different from this "clear"
>>>> operation.
>>>
>>> I agree that these names could have been better.
>>>
>>> s/clear_trbe_state/trbe_reset_perf_state - Clears TRBE from current perf config
>>> s/set_trbe_state/trbe_prepare_perf_state - Prepares TRBE for the next perf config
>>
>> Please don't tie them to "perf". This is pure hardware configuration, not perf.
>
> Okay.
>
>>
>> Also, I wonder if we need a separate "set_trbe_state". Could we not initialize the LIMITR
>> at one go ?
>
> There are some limitations which could prevent that.
>
>>
>> i.e, do something like :
>>
>> set_trbe_limit_pointer(limit, mode) ?
>>
>> where it sets all the fields of limit pointer. Also, you may want to document the mode we
>> choose for TRBE. i.e, FILL STOP mode for us to collect the trace.
>
> Sure, will document the TRBE mode being choosen here.
>
>>
>>>
>>>
>>>>
>>>> I would rather open code this with a write of 0 to trbsr in the caller.
>>>>
>>>>> +{
>>>>> +    u64 trbsr = read_sysreg_s(SYS_TRBSR_EL1);
>>>>> +
>>>>> +    WARN_ON(is_trbe_enabled());
>>>>> +    trbsr &= ~TRBSR_IRQ;
>>>>> +    trbsr &= ~TRBSR_TRG;
>>>>> +    trbsr &= ~TRBSR_WRAP;
>>>>> +    trbsr &= ~(TRBSR_EC_MASK << TRBSR_EC_SHIFT);
>>>>> +    trbsr &= ~(TRBSR_BSC_MASK << TRBSR_BSC_SHIFT);
>>>>> +    trbsr &= ~(TRBSR_FSC_MASK << TRBSR_FSC_SHIFT);
>>>>
>>>> BSC and FSC are the same fields under MSS, with their meanings determined by the EC field.
>>>
>>> Could just drop the FSC part if required.
>>>
>>>>
>>>> Could we simply write 0 to the register ?
>>>
>>> I would really like to avoid that. This function clearly enumerates all
>>> individual bit fields being cleared for resetting as well as preparing
>>> the TRBE for the next perf session. Converting this into a 0 write for
>>> SYS_TRBSR_EL1 sounds excessive and the only thing it would save is the
>>> register read.
>>
>>>
>>>>
>>>>> +    write_sysreg_s(trbsr, SYS_TRBSR_EL1);
>>>>> +}
>>>>> +
>>>>> +static void set_trbe_state(void)
>>>>> +{
>>>>> +    u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
>>>>> +
>>>>> +    trblimitr &= ~TRBLIMITR_NVM;
>>>>> +    trblimitr &= ~(TRBLIMITR_FILL_MODE_MASK << TRBLIMITR_FILL_MODE_SHIFT);
>>>>> +    trblimitr &= ~(TRBLIMITR_TRIG_MODE_MASK << TRBLIMITR_TRIG_MODE_SHIFT);
>>>>> +    trblimitr |= (TRBE_FILL_STOP & TRBLIMITR_FILL_MODE_MASK) << TRBLIMITR_FILL_MODE_SHIFT;
>>>>> +    trblimitr |= (TRBE_TRIGGER_IGNORE & TRBLIMITR_TRIG_MODE_MASK) << TRBLIMITR_TRIG_MODE_SHIFT;
>>>>> +    write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1);
>>>>
>>>> Do we need to read-copy-update here ? Could we simply write 0 ?
>>>> Same as above comment, could we not simply opencode it at the caller ?
>>>> Clearly the names don't help.
>>>
>>> Will change the names as proposed or something better. But lets leave
>>> these functions as is. Besides TRBE_TRIGGER_IGNORE also has a positive
>>> value (i.e 3), writing all 0s into SYS_TRBLIMITR_EL1 will not be ideal.
>>>
>>
>> The point is, we don't need to preserve the values for LIMITR. Also see my comment
>> above, for folding this to set_trbe_limit_pointer(). In any case, I don't think
>> we should rely on the values of fields we change. So it is safer and cleaner to
>> set set all the bits for LIMITR, including the LIMIT address in one go, without
>> ready-copy-update.
>
> TRBE needs to be disabled (which is also in the LIMIT register) before we can update
> any other fields in the LIMIT register. So there is already an order dependency here.
> Looking at the function trbe_enable_hw(), it follows something like
>
> 1. Clear and set the TRBE mode - followed by an isb()
> 2. Update the TRBE pointers - followed by an isb()
> 3. Set it rolling - followed by TSB_CSYNC
>
> static void trbe_enable_hw(struct trbe_buf *buf)
> {
>
> [Software checks]
> WARN_ON(buf->trbe_write < buf->trbe_base);
> WARN_ON(buf->trbe_write >= buf->trbe_limit);
>
> [Disable TRBE in the limit register]
> set_trbe_disabled();
>

We need an isb() here.

> [Clears TRBE status register]
> trbe_reset_perf_state();

Please be explicit here. Make the function name reflect the fact that
we are simply clearing the status register and nothing related to perf.

>
> [Configures TRBE mode in the limit register]
> trbe_prepare_perf_state();

This is unnecessarily introducing a dependency not enforced by the HW.
You could program the LIMIT register with all the setting, mode, limit
and *enable TBRE* once we have programmed base and write pointer at one
shot.

>
> isb();

Drop the ISB

>
> [Update all required pointers]
> set_trbe_base_pointer(buf->trbe_base);
> set_trbe_limit_pointer(buf->trbe_limit);

As mentioned above, this could be done in set_trbe_enabled()

> set_trbe_write_pointer(buf->trbe_write);
> isb();
>
> [Set it rolling]
>
> [Update TRBE status register stop bit]
> set_trbe_running();

This doesn't have any significance with Hardware. It is a status bit
from the HW, which is writable only for "state" save/restore, when
switching between contexts. Otherwise, this write doesn't do anything.
So, please combine this with the clear_status operation above.

>
> [Update TRBE limit register enable bit]
> set_trbe_enabled();

Here we could set all the fileds of the LIMIT register, followed by
an isb()

Kind regards
Suzuki

2021-01-07 14:08:59

by Suzuki K Poulose

[permalink] [raw]

Subject: Re: [PATCH 11/11] dts: bindings: Document device tree binding for Arm TRBE

On 1/4/21 3:44 AM, Anshuman Khandual wrote:
>
> On 1/3/21 10:35 PM, Rob Herring wrote:
>> On Wed, Dec 23, 2020 at 03:33:43PM +0530, Anshuman Khandual wrote:
>>> This patch documents the device tree binding in use for Arm TRBE.
>>>
>>> Cc: [email protected]
>>> Cc: Mathieu Poirier <[email protected]>
>>> Cc: Mike Leach <[email protected]>
>>> Cc: Suzuki K Poulose <[email protected]>
>>> Signed-off-by: Anshuman Khandual <[email protected]>
>>> ---
>>> Changes in V1:
>>>
>>> - TRBE DT entry has been renamed as 'arm, trace-buffer-extension'
>>>
>>> Documentation/devicetree/bindings/arm/trbe.txt | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/arm/trbe.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/trbe.txt b/Documentation/devicetree/bindings/arm/trbe.txt
>>> new file mode 100644
>>> index 0000000..001945d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/trbe.txt
>>> @@ -0,0 +1,20 @@
>>> +* Trace Buffer Extension (TRBE)
>>> +
>>> +Trace Buffer Extension (TRBE) is used for collecting trace data generated
>>> +from a corresponding trace unit (ETE) using an in memory trace buffer.
>>> +
>>> +** TRBE Required properties:
>>> +
>>> +- compatible : should be one of:
>>> + "arm,trace-buffer-extension"
>>> +
>>> +- interrupts : Exactly 1 PPI must be listed. For heterogeneous systems where
>>> + TRBE is only supported on a subset of the CPUs, please consult
>>> + the arm,gic-v3 binding for details on describing a PPI partition.
>>> +
>>> +** Example:
>>> +
>>> +trbe {
>>> + compatible = "arm,trace-buffer-extension";
>>> + interrupts = <GIC_PPI 15 IRQ_TYPE_LEVEL_HIGH>;
>>
>> If only an interrupt, then could just be part of ETE? If not, how is
>> this hardware block accessed? An interrupt alone is not enough unless
>> there's some architected way to access.
>
> TRBE hardware block is accessed via respective new system registers but the
> PPI number where the IRQ will be triggered for various buffer events, would
> depend on the platform as defined in the SBSA.

That is correct. TRBE is accessed via CPU system registers. The IRQ is specifically
for the TRBE unit to handle buffer overflow situations and other errors in the
buffer handling. Please include this information in the description section of
the bindings.

Also, it may be worth switching this to yaml format.

Suzuki