lkml.org 
[lkml]   [2023]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] stm: class: Add MIPI OST protocol support
From
Hi Alexande,

On 3/3/2023 2:05 AM, Alexander Shishkin wrote:
> Mao Jinlong <quic_jinlmao@quicinc.com> writes:
>
>> Add MIPI OST protocol support for stm to format the traces.
> Missing an explanation of what OST is, what it's used for, how it is
> different from the SyS-T and others.
I will updae the explanation in next version.
>
>> Framework copied from drivers/hwtracing/stm.p-sys-t.c as of
> You mean stm/p_sys-t.c. Also, it's not a framework, it's a driver.

The driver refers to code structure of p_sys-t driver. So, add this
comments after
internal review.

>
>> commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
>> support").
> Why is this significant?
>
>> diff --git a/drivers/hwtracing/stm/p_ost.c b/drivers/hwtracing/stm/p_ost.c
>> new file mode 100644
>> index 000000000000..2ca1a3fda57f
>> --- /dev/null
>> +++ b/drivers/hwtracing/stm/p_ost.c
>> @@ -0,0 +1,95 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit d69d5e83110f
>> + * ("stm class: Add MIPI SyS-T protocol support").
> Same as in the commit message.
>
> [...]
>
>> +#define OST_TOKEN_STARTSIMPLE (0x10)
>> +#define OST_VERSION_MIPI1 (0x10 << 8)
>> +#define OST_ENTITY_FTRACE (0x01 << 16)
>> +#define OST_CONTROL_PROTOCOL (0x0 << 24)
> These could use an explanation.
I will add the explanation.
>> +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
>> + OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)
> Does this mean that everything is ftrace? Because it's not.
Only ftrace is supported in p_ost now. Other header type will be added
later.
>
>> +
>> +#define STM_MAKE_VERSION(ma, mi) ((ma << 8) | mi)
>> +#define STM_HEADER_MAGIC (0x5953)
>> +
>> +static ssize_t notrace ost_write(struct stm_data *data,
>> + struct stm_output *output, unsigned int chan,
>> + const char *buf, size_t count)
>> +{
>> + unsigned int c = output->channel + chan;
>> + unsigned int m = output->master;
>> + const unsigned char nil = 0;
>> + u32 header = DATA_HEADER;
>> + u8 trc_hdr[24];
>> + ssize_t sz;
>> +
>> + /*
>> + * STP framing rules for OST frames:
>> + * * the first packet of the OST frame is marked;
>> + * * the last packet is a FLAG.
> Which in your case is also timestamped.
I will add the comments.
>
>> + */
>> + /* Message layout: HEADER / DATA / TAIL */
>> + /* HEADER */
>> +
>> + sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
>> + 4, (u8 *)&header);
> The /* HEADER */ comment applies to the above line, so it should
> probably be directly before it.
Got it.
>
>> + if (sz <= 0)
>> + return sz;
>> + *(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
>> + *(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
>> + *(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
>> + *(uint64_t *)(trc_hdr + 8) = sched_clock();
> Why sched_clock()? It should, among other things, be called with
> interrupts disabled, which is not the case here.
I will check. If it is not necessary here, I will remove it.
>
>> + *(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());
> Is there a reason why trc_hdr is not a struct?
No particular reason here.
>
> Thanks,
> --
> Alex

\
 
 \ /
  Last update: 2023-03-27 00:47    [W:0.139 / U:0.652 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site